Build throwing errors after TypeScript v2 release, yet seen as a green build

[andrewconnell]

Errors with the build… looks like it happened when v2 TypeScript was released. What’s strange is errors in tests aren’t failing the build so need to dig into this. Appears it all started about 8 days ago… if you look at this build, it shows that there were errors in the build, yet build is green.

Ref: https://circleci.com/gh/ngOfficeUIFabric/ng-officeuifabric/184

[andrewconnell]

I've isolated the issue down to something we were getting away with that wasn't allowed, but upgrading to the latest TypeScript & ts-lint exposed it. The fix I applied in @0f1749a22e8f1cef2880ed3dba1776262d54bbe8 got the project to compile, but the tests are throwing errors (see CircleCI build 184).

This doesn't address WHY CircleCI was reporting green builds when there were errors in the test runs, which I'm trying to isolate / ID...

The problem was that we were setting the width of the window to test overflow with the breadcrumb with:

$window.innerWidth = 800;

... but that is a read-only property & this was failing when we transpiled the code (see CircleCI build 183).

So I changed it to use the resize function ($window.resizeTo(800, $window.innerHeight);)... that fixed the compile, but when the test runs, it's clearly not setting the window size correctly thus triggering the errors when running tests (5 of them).

Oh, and I know I screwed up when I fixed the code as I essentially resized it using .resizeTo(width, width)... I'll fix that, but doesn't change the fact the window isn't resizing.

I've spent numerous hours trying to figure out how to resize the window correctly within the test but so far no joy... every time I set it and try different things to get it to apply, the dimensions are still the same. I've seen some references to people creating special Karma configuration runs, but that's not what I want as we don't want to do it for all tests... just specific tests.

Looking for help here... anyone know how to do this as I'm at a loss. My Google-foo seems to be failing me too. The breadcrumb specs need to test for overflow which is driven by the window size... maybe another option is to consider not resizing the window, but modifying the directive so it only checks for the element size rather than the window size? This makes sense to me since if the breadcrumb was within a container, it should base it's overflow on the container size, not the full window size... right?

Folks who've done any work on the breadcrumb: /cc @andikrueger @rolandoldengarm @jjczopek

[s-KaiNet]

For testing navbar in mobile mode I've used the following:

beforeAll(() => {
      originalWidth = jQuery(document.body).css('width');
      jQuery(document.body).css('width', '470px');
      jQuery(window).trigger('resize');
    });
afterAll(() => {
      jQuery(document.body).css('width', originalWidth);
      jQuery(window).trigger('resize');
    });

You can take a look under navbar specs
I believe this should work, since navbar tests are passing.

[andrewconnell]

Awesome... thanks @s-KaiNet ... I'll take a look.

I did figure out why the failed tests were not generating red builds... gulp was always returning a 0 return code! How we got this far like that I don't know, or if some dependency was recently "fixed" so it did what it should have done... but modified the test gulp task so it returns either a zero (on good) or non-zero (on bad) test runs.

[jjczopek]

@andrewconnell breadcrumb dependency on window object AFAIR was implemented the same it was in native Office UI Fabric back in that time. We can move out from that dependency, just we have to remember it's a braking change for anyone who already is using this component.

The other thing is, how we would actually like to get rid of window dependency:

• introduce a wrapper element inside the directive, with hard-coding it's width to 100%, and base the calculations on this elements width, or
• base the calculations based on parent element width and developer is responsible for wrapping the directive.

I think the latter gives a bit more flexibility, because then developer can style wrapper to be 100% size of the window if that's what he wants or any other custom size.

[andrewconnell]

I'm not sure we need to go that far... esp since it's a breaking change. Considering the component works just fine today, I don't think we need to change it.

I've had a dialog in the Slack general channel about this today. Thinking more, the resize event is really not something you want to unit test for, rather I think it's more of an integration test and thus, warrants protractor. That's starting to bite off a whole different animal for our project so instead, I think what I'll do is modify the tests to instead call the recalculation method that's used to figure out what items should be visible & what should be in the overflow. I can test for those. The unit tests will just ignore the resize aspect.

[jjczopek]

sounds legit. I have finished fixing searchbox but will wait for your fixes to put proper PR for this.

[andrewconnell]

Feel free to submit PR... as long as your PR has errors for vetting & tests for the breadcrumb directive, it should be fine. The dev branch has the updates to the gulp test task that had to get fixed... so as long as you resync your local fork's dev branch & branch from that, you should be ok on the PR.

The only outstanding thing I have to do is fix the breadcrumb tests to get back to green builds.