destroy testing modules during resetTestingModule
#38336
Conversation
|
Hi, can |
d7d142b to
b803904
Compare
b803904 to
1490c1d
Compare
63e25ad to
7c48418
Compare
There is currently a known issue with the framework when testing that leaks style. See angular#31834 A possible temporary workaround is to manually call the service that adds/aremoves the style. See angular#31834 (comment) This workaround will be unnecessary when angular#38336 lands. This speeds up the `ng test` task locally by ~30%: - before: Executed 649 of 652 (skipped 3) SUCCESS (24.161 secs / 20.072 secs) - after: Executed 649 of 652 (skipped 3) SUCCESS (17.854 secs / 15.584 secs)
There is currently a known issue with the framework when testing that leaks style. See angular#31834 A possible temporary workaround is to manually call the service that adds/aremoves the style. See angular#31834 (comment) This workaround will be unnecessary when angular#38336 lands. This speeds up the `ng test` task locally by ~30%: - before: Executed 649 of 652 (skipped 3) SUCCESS (24.161 secs / 20.072 secs) - after: Executed 649 of 652 (skipped 3) SUCCESS (17.854 secs / 15.584 secs)
There was a problem hiding this comment.
It looks like CircleCI is requiring this PR be rebased:
CircleCI config on master has been modified since commit 250e299,
which this PR is based on.
Please rebase the PR on master after fetching from upstream.
Rebase instructions for PR Author, please run the following commands:
git fetch upstream master;
git checkout shared-styles-cleanup;
git rebase upstream/master;
git push --force-with-lease;
| schema: DomElementSchemaRegistry, private component: RendererType2) { | ||
| super(eventManager, document, ngZone, schema); | ||
| console.log('renderer created'); |
There was a problem hiding this comment.
Was this just for debugging? Can it be removed now?
7c48418 to
0ce8927
Compare
|
@AndrewKushnir can you describe how this change is breaking? |
|
@elesueur given that there was no cleanup of shared styles, tests might be written in a way where they depend on the presence of some CSS rules added during previous tests. With the proposed cleanup, such tests will start to fail (so this is a breaking change). We've confirmed that by running tests in Google's codebase. Before merging this change it's important to estimate potential risks and come up with proper rollout plan/communication. @josephperrott please correct me if I'm wrong or if I'm missing something. |
|
That is correct, I would only add that it could go even further and maintain the state in other ways between tests. Since modules are not torn down, things like the state of a service can leak between tests. |
As a maintainer of several large Angular apps, I would want to learn about which tests are relying on pollution (and fix them) and get the performance improvement rather than wait for a 'less breaky' solution that hides the pollution and leaves me with slow tests :) |
…ule` Previously, the reference to the test `NgModuleRef` was set to null when the testing module was reset via `resetTestingModule`. While this clears the reference to the specific instance of `NgModuleRef`, it does not properly break down/destroy the established injectibles within the `TestBed`. By calling `destroy` on the `_moduleRef` instance, the entire established structue is correctly torn down to truly reset what the module has created.
0ce8927 to
2505cf2
Compare
|
Will be superseded by #42566. |
|
Closing in favor of #42566 |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
See individual commits.
Fixes #18831