build: report ngOnDestroy exceptions in unti tests #5036

devversion · 2017-06-09T00:06:53Z

Monkey-patches TestBed.resetTestingModule such that any errors that occur during component destruction are thrown instead of silently logged. Also runs TestBed.resetTestingModule after each unit test.

Without this patch, the combination of two behaviors is problematic for Angular Material:

TestBed.resetTestingModule catches errors thrown on fixture destruction and logs them without the errors ever being thrown. This means that any component errors that occur in ngOnDestroy can encounter errors silently and still pass unit tests.
TestBed.resetTestingModule is only called before a test is run, meaning that even if the aforementioned errors were thrown, they would be reported for the wrong test (the test that's about to start, not the test that just finished).

jelbourn · 2017-06-09T16:03:50Z

src/lib/datepicker/datepicker.ts

@@ -257,7 +257,7 @@ export class MdDatepicker<D> implements OnDestroy {
      this._ngZone.onStable.first().subscribe(() => this._popupRef.updatePosition());
    }

-    this._popupRef.backdropClick().first().subscribe(() => this.close());
+    this._popupRef.backdropClick().subscribe(() => this.close());


Should already be fixed in master
f89c6db

jelbourn · 2017-06-09T16:08:47Z

test/karma-test-shim.js

+}
+
+/** Applies patches and workarounds to Angular's testing package. */
+function applyAngularTestingPatches(testBed) {


How about patchTestModuleToDestroyFixturesAfterEveryTest?

/** * Monkey-patches TestBed.resetTestingModule such that any errors that occur during component * destruction are thrown instead of silently logged. Also runs TestBed.resetTestingModule after * each unit test. * * Without this patch, the combination of two behaviors is problematic for Angular Material: * - TestBed.resetTestingModule catches errors thrown on fixture destruction and logs them without * the errors ever being thrown. This means that any component errors that occur in ngOnDestroy * can encounter errors silently and still pass unit tests. * - TestBed.resetTestingModule is only called *before* a test is run, meaning that even *if* the * aforementioned errors were thrown, they would be reported for the wrong test (the test that's * about to start, not the test that just finished). */ function patchTestBedToDestroyFixturesAfterEveryTest() { }

Feels a bit long but should be fine for the shims.

jelbourn · 2017-06-09T16:22:49Z

test/karma-test-shim.js

+function applyAngularTestingPatches(testBed) {
+  // Original resetTestingModule function of the TestBed.
+  var _resetTestingModule = testBed.resetTestingModule;
+


Maybe add something like...

if (!testBed._activeFixtures) { throw Error('The monkey-patch for TestBed.resetTestingModule has failed ' + 'because a new version of Angular has removed the _activeFixtures member. ' + 'This needs to be fixed in karma-test-shim.js'); }

I don't think this is necessary. If it will fail we will immediately see that the shims cause it to fail.

The error that would be thrown is Cannot read property 'forEach' of undefined. We have much more knowledge about this failure state, so we can help whoever is debugging this in the future by making a much more helpful message.

That's true, but what I mean is that it's very unlikely that the _activeFixtures property will be removed or whatever.

If it really comes to an error you will see the file that failed and the according line number.

Would be test/karma-test-shim.js:XXX

jelbourn · 2017-06-09T16:29:36Z

test/karma-test-shim.js

+    }
+  };
+
+  // Angular's testing package resets the testing module before each test. This is bad because


"This is bad" -> "This doesn't work well for us"

Yeah makes sense.

jelbourn · 2017-06-09T16:30:14Z

Also the name of the PR / commit should reflect the content of the karma-shim change

devversion · 2017-06-09T16:59:15Z

I only mentioned the datepicker because if it gets merged the changelog should only include that the datepicker bug was fixed.

But since the fix is merged already it makes sense to make it a chore: now.

Monkey-patches TestBed.resetTestingModule such that any errors that occur during component destruction are thrown instead of silently logged. Also runs TestBed.resetTestingModule after each unit test. Without this patch, the combination of two behaviors is problematic for Angular Material: - TestBed.resetTestingModule catches errors thrown on fixture destruction and logs them without the errors ever being thrown. This means that any component errors that occur in ngOnDestroy can encounter errors silently and still pass unit tests. - TestBed.resetTestingModule is only called *before* a test is run, meaning that even *if* the aforementioned errors were thrown, they would be reported for the wrong test (the test that's about to start, not the test that just finished).

devversion · 2017-06-09T18:11:58Z

Addressed feedback.

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-07T16:03:00Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

devversion added the pr: needs review label Jun 9, 2017

devversion assigned jelbourn Jun 9, 2017

devversion requested a review from jelbourn June 9, 2017 00:06

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 9, 2017

devversion force-pushed the fix/datepicker-destroy-error branch from 8f1f428 to 3b49661 Compare June 9, 2017 00:13

jelbourn reviewed Jun 9, 2017

View reviewed changes

devversion force-pushed the fix/datepicker-destroy-error branch from 3b49661 to 324b926 Compare June 9, 2017 18:08

devversion force-pushed the fix/datepicker-destroy-error branch from 324b926 to 8cde483 Compare June 9, 2017 18:11

devversion changed the title ~~fix(datepicker): rxjs throws error on component destroy~~ build: report ngOnDestroy exceptions in unti tests Jun 9, 2017

jelbourn approved these changes Jun 9, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 9, 2017

andrewseguin merged commit 21f764c into angular:master Jun 9, 2017

devversion deleted the fix/datepicker-destroy-error branch November 11, 2017 10:23

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: report ngOnDestroy exceptions in unti tests #5036

build: report ngOnDestroy exceptions in unti tests #5036

Uh oh!

devversion commented Jun 9, 2017 •

edited

Loading

Uh oh!

jelbourn Jun 9, 2017

Uh oh!

jelbourn Jun 9, 2017

Uh oh!

devversion Jun 9, 2017

Uh oh!

jelbourn Jun 9, 2017

Uh oh!

devversion Jun 9, 2017

Uh oh!

jelbourn Jun 9, 2017

Uh oh!

devversion Jun 9, 2017

Uh oh!

jelbourn Jun 9, 2017

Uh oh!

devversion Jun 9, 2017

Uh oh!

jelbourn commented Jun 9, 2017

Uh oh!

devversion commented Jun 9, 2017

Uh oh!

devversion commented Jun 9, 2017

Uh oh!

jelbourn left a comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Uh oh!

build: report ngOnDestroy exceptions in unti tests #5036

build: report ngOnDestroy exceptions in unti tests #5036

Uh oh!

Conversation

devversion commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Jun 9, 2017

Uh oh!

devversion commented Jun 9, 2017

Uh oh!

devversion commented Jun 9, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Uh oh!

devversion commented Jun 9, 2017 •

edited

Loading