feat(core): add opt-in test module teardown configuration #42566

crisbeto · 2021-06-13T08:09:23Z

We currently have two long-standing issues related to how TestBed tests are torn down:

The dynamically-created test module isn't going to be destroyed, preventing the ngOnDestroy hooks on providers from running and keeping the component style nodes in the DOM.
The test root elements aren't going to be removed from the DOM. Instead, they will be removed whenever another test component is created.

By themselves, these issues are easy to resolve, but given how long they've been around, there are a lot of unit tests out there that depend on the broken behavior.

These changes address the issues by introducing APIs that allow users to opt into the correct test teardown behavior either at the application level via TestBed.initTestEnvironment or the test suite level via TestBed.configureTestingModule.

At the moment, the new teardown behavior is opt-in, but the idea is that we'll eventually make it opt-out before removing the configuration altogether.

Fixes #18831.

crisbeto · 2021-06-13T08:13:31Z

packages/core/test/test_bed_spec.ts

+    expect(fixtureDocument.body.contains(fixture.nativeElement)).toBe(false);
+  });
+
+  it('should re-throw errors that were thrown during fixture cleanup', () => {


One note here: the original design only called for test module errors to be re-thrown, not test component, but I decided to make this change, because I was looking at how the Components repo is working around this and I realized that not throwing errors during test component destruction can still lead to difficult to trace down bugs. For reference: https://github.com/angular/components/blob/master/test/karma-test-shim.js#L61

AndrewKushnir

Looks great, thanks @crisbeto! I've just left a few comments, please have a look when you get a chance.

packages/core/testing/src/r3_test_bed.ts

packages/core/testing/src/test_hooks.ts

packages/core/testing/src/r3_test_bed.ts

AndrewKushnir · 2021-06-15T01:07:10Z

packages/core/testing/src/test_bed_common.ts

+ * @publicApi
+ */
+export interface ModuleTeardownOptions {
+  /** Whether the test module should be destroyed after every test. */


It'd be great to mention a default value here too.

I avoid mentioning default values in docs, because they inevitably go out of sync when the default changes.

Ok, got it. I just noticed that you've added the default value description to the rethrowErrors field, but not this one.

I don't anticipate for the rethrowErrors default value to change, whereas the teardown one will.

AndrewKushnir · 2021-06-15T01:09:47Z

packages/core/testing/src/test_bed_common.ts

+}
+
+/**
+ * Object used to configure the test module teardown behavior in `TestBed`.


I think we'd need to add more info here to explain why these options exist and provide recommendations on when they should be used/changed and what the plan is (that these options will eventually be deprecated and removed, so we do not encourage using them unless it's absolutely needed and blocking tests).

The idea was to have a silent rollout of this in 12.1.0 and then add a page in the docs similar to https://angular.io/guide/migration-update-libraries-tslib.

Thanks for additional info, I'd propose adding a couple sentences to describe that:

Suggested change

* Object used to configure the test module teardown behavior in `TestBed`.

* Object used to configure the test module teardown behavior in `TestBed`.

* This option is exposed as a public API to allow switching to the legacy TestBed behavior temporarily

* (where testing module was not removed as a part of the test teardown process) in case your tests rely on

* this old behavior. These config options will be deprecated and removed in the future versions of Angular.

I'm not sure that people who aren't familiar with the TestBed internals would know what this is referring to.

We can update the message to make it more user-friendly. I'd like to include some information about the fact that these config options are temporary and developers should not rely on them.

IMO we should capture the fact that it's temporary when we eventually add the page on AIO. Also the plan is to provide schematics that add the parameter and then remove it once we've changed the default value so hopefully people won't have to deal with it themselves.

The ModuleTeardownOptions would become discoverable on the AIO once this PR is merged, see
https://pr42566-893884a.ngbuilds.io/api/core/testing/ModuleTeardownOptions.

I meant that providing all the context behind the option would take a few paragraphs of text which seems like overkill for an API doc. E.g. the design doc for these changes takes 170 words to explain the issues.

josephperrott

LGTM

packages/core/testing/src/test_hooks.ts

crisbeto · 2021-06-15T18:11:03Z

@AndrewKushnir I've addressed the feedback.

@josephperrott I've pushed a fix for it. I had to revert one place where I tried to make something a couple lines shorter, but it ended up causing the error.

AndrewKushnir · 2021-06-15T21:06:13Z

packages/core/testing/src/r3_test_bed.ts

+    }
+
+    // Otherwise use the configured behavior or default to rethrowing.
+    return instanceOptions?.rethrowErrors ?? environmentOptions?.rethrowErrors ?? true;


Would it make sense to extract this default value to a const too (similar to TEARDOWN_TESTING_MODULE_ON_DESTROY_DEFAULT)?

I think the constant made sense for the teardown option since we're going to change it over time. I don't anticipate for the error rethrowing default behavior to change.

AndrewKushnir · 2021-06-15T21:11:45Z

packages/core/testing/src/test_bed_common.ts

+ * @publicApi
+ */
+export interface ModuleTeardownOptions {
+  /** Whether the test module should be destroyed after every test. */


Ok, got it. I just noticed that you've added the default value description to the rethrowErrors field, but not this one.

AndrewKushnir · 2021-06-15T21:21:00Z

packages/core/testing/src/test_bed_common.ts

+}
+
+/**
+ * Object used to configure the test module teardown behavior in `TestBed`.


Thanks for additional info, I'd propose adding a couple sentences to describe that:

Suggested change

* Object used to configure the test module teardown behavior in `TestBed`.

* Object used to configure the test module teardown behavior in `TestBed`.

* This option is exposed as a public API to allow switching to the legacy TestBed behavior temporarily

* (where testing module was not removed as a part of the test teardown process) in case your tests rely on

* this old behavior. These config options will be deprecated and removed in the future versions of Angular.

packages/core/testing/src/r3_test_bed.ts

atscott

reviewed-for: public-api

AndrewKushnir

Reviewed-for: public-api

mary-poppins · 2021-06-16T19:20:39Z

You can preview 0a272fd at https://pr42566-0a272fd.ngbuilds.io/.
You can preview dd2cb6e at https://pr42566-dd2cb6e.ngbuilds.io/.
You can preview b7a29d2 at https://pr42566-b7a29d2.ngbuilds.io/.
You can preview 893884a at https://pr42566-893884a.ngbuilds.io/.

mary-poppins · 2021-06-16T19:22:59Z

You can preview 0a272fd at https://pr42566-0a272fd.ngbuilds.io/.
You can preview dd2cb6e at https://pr42566-dd2cb6e.ngbuilds.io/.
You can preview b7a29d2 at https://pr42566-b7a29d2.ngbuilds.io/.
You can preview 893884a at https://pr42566-893884a.ngbuilds.io/.

jessicajaniuk

LGTM 🍪

Reviewed-for: public-api

We currently have two long-standing issues related to how `TestBed` tests are torn down: 1. The dynamically-created test module isn't going to be destroyed, preventing the `ngOnDestroy` hooks on providers from running and keeping the component `style` nodes in the DOM. 2. The test root elements aren't going to be removed from the DOM. Instead, they will be removed whenever another test component is created. By themselves, these issues are easy to resolve, but given how long they've been around, there are a lot of unit tests out there that depend on the broken behavior. These changes address the issues by introducing APIs that allow users to opt into the correct test teardown behavior either at the application level via `TestBed.initTestEnvironment` or the test suite level via `TestBed.configureTestingModule`. At the moment, the new teardown behavior is opt-in, but the idea is that we'll eventually make it opt-out before removing the configuration altogether. Fixes angular#18831.

Angular 12.1.0 includes [opt-in automatic test module teardown](angular/angular#42566) which is similar to our logic of destroying the current fixture. These changes clean up our monkey patches and use the new option instead.

In version 12.1, the framework added the `destroyAfterEach` an opt-in feature that improves tests performance and also addresses two long-standing issues. The idea, is to have this enabled by default in the future. Related PR: angular/angular#42566 Closes #21280

Angular 12.1.2 includes [opt-in automatic test module teardown](angular/angular#42566) which is similar to our logic of destroying the current fixture. These changes clean up our monkey patches and use the new option instead.

Angular 12.1.2 includes [opt-in automatic test module teardown](angular/angular#42566) which is similar to our logic of destroying the current fixture. These changes clean up our monkey patches and use the new option instead. (cherry picked from commit d090617)

angular-automatic-lock-bot · 2021-07-18T16:06:02Z

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.}

google-cla bot added the cla: yes label Jun 13, 2021

crisbeto commented Jun 13, 2021

View reviewed changes

crisbeto mentioned this pull request Jun 13, 2021

destroy testing modules during resetTestingModule #38336

Closed

crisbeto force-pushed the testbed-teardown branch 2 times, most recently from 0a5748c to dd2cb6e Compare June 13, 2021 08:33

ngbot bot modified the milestone: Backlog Jun 13, 2021

crisbeto marked this pull request as ready for review June 13, 2021 09:09

pullapprove bot requested review from alxhub and AndrewKushnir June 13, 2021 09:10

josephperrott self-requested a review June 14, 2021 19:24

AndrewKushnir reviewed Jun 15, 2021

View reviewed changes

josephperrott approved these changes Jun 15, 2021

View reviewed changes

josephperrott reviewed Jun 15, 2021

View reviewed changes

packages/core/testing/src/test_hooks.ts Show resolved Hide resolved

crisbeto force-pushed the testbed-teardown branch from dd2cb6e to b7a29d2 Compare June 15, 2021 18:10

crisbeto force-pushed the testbed-teardown branch from b7a29d2 to 893884a Compare June 15, 2021 18:24

AndrewKushnir approved these changes Jun 15, 2021

View reviewed changes

pullapprove bot requested review from atscott, jessicajaniuk and petebacondarwin June 15, 2021 21:23

atscott approved these changes Jun 16, 2021

View reviewed changes

pullapprove bot requested a review from IgorMinar June 16, 2021 16:40

AndrewKushnir approved these changes Jun 16, 2021

View reviewed changes

AndrewKushnir added aio: preview and removed aio: preview labels Jun 16, 2021

jessicajaniuk approved these changes Jun 16, 2021

View reviewed changes

jessicajaniuk removed request for petebacondarwin, IgorMinar and alxhub June 16, 2021 20:15

crisbeto added 3 commits June 17, 2021 19:02

fixup! feat(core): add opt-in test module teardown configuration

6486023

fixup! feat(core): add opt-in test module teardown configuration

e8f32a2

crisbeto force-pushed the testbed-teardown branch from 893884a to e8f32a2 Compare June 17, 2021 17:05

crisbeto removed the aio: preview label Jun 17, 2021

dylhunn removed the action: presubmit The PR is in need of a google3 presubmit label Jun 17, 2021

dylhunn closed this in 873229f Jun 17, 2021

crisbeto mentioned this pull request Jun 25, 2021

build: enable automatic test module teardown angular/components#23070

Closed

NetanelBasal mentioned this pull request Jun 25, 2021

Upgrade to v12 ngneat/spectator#442

Closed

alan-agius4 mentioned this pull request Jul 7, 2021

feat(@schematics/angular): destroy test module after every test angular/angular-cli#21300

Merged

ersimont mentioned this pull request Jul 13, 2021

[ng-dev] trimLeftoverStyles() trims things added by ionic or fontawesome simontonsoftware/s-libs#34

Closed

crisbeto mentioned this pull request Jul 15, 2021

build: enable automatic test module teardown angular/components#23176

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add opt-in test module teardown configuration #42566

feat(core): add opt-in test module teardown configuration #42566

crisbeto commented Jun 13, 2021 •

edited

Loading

crisbeto Jun 13, 2021 •

edited

Loading

AndrewKushnir left a comment

AndrewKushnir Jun 15, 2021

crisbeto Jun 15, 2021

AndrewKushnir Jun 15, 2021

crisbeto Jun 16, 2021

AndrewKushnir Jun 15, 2021

crisbeto Jun 15, 2021

AndrewKushnir Jun 15, 2021

crisbeto Jun 16, 2021

AndrewKushnir Jun 16, 2021

crisbeto Jun 16, 2021

AndrewKushnir Jun 16, 2021

crisbeto Jun 16, 2021

josephperrott left a comment

crisbeto commented Jun 15, 2021

AndrewKushnir Jun 15, 2021

crisbeto Jun 16, 2021

AndrewKushnir Jun 15, 2021

AndrewKushnir Jun 15, 2021

atscott left a comment

AndrewKushnir left a comment

mary-poppins commented Jun 16, 2021

mary-poppins commented Jun 16, 2021

jessicajaniuk left a comment

angular-automatic-lock-bot bot commented Jul 18, 2021

- * Object used to configure the test module teardown behavior in `TestBed`.
+ * Object used to configure the test module teardown behavior in `TestBed`.
+ * This option is exposed as a public API to allow switching to the legacy TestBed behavior temporarily
+ * (where testing module was not removed as a part of the test teardown process) in case your tests rely on
+ * this old behavior. These config options will be deprecated and removed in the future versions of Angular.

feat(core): add opt-in test module teardown configuration #42566

feat(core): add opt-in test module teardown configuration #42566

Conversation

crisbeto commented Jun 13, 2021 • edited Loading

crisbeto Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephperrott left a comment

Choose a reason for hiding this comment

crisbeto commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

mary-poppins commented Jun 16, 2021

mary-poppins commented Jun 16, 2021

jessicajaniuk left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Jul 18, 2021

crisbeto commented Jun 13, 2021 •

edited

Loading

crisbeto Jun 13, 2021 •

edited

Loading