Dialog is not removed during tests, leading to order-dependent tests

[jbeder]

I have a set of screenshot tests using Angular Catalyst that pass when run A, B but fail when run B, A; the failure is that the screenshot for A, when run after B, shows the shadow of the dialog that was created for B. (I'm not linking to them because they're internal to Google.)

If I add:

afterEach(async () => {
  const dialog = await getHarness(MatDialogHarness, {useDocumentRoot: true});
  await dialog.close();
});

to test B, then they pass in either order. The test infrastructure should clean this up automatically to keep the tests hermetic.

@josephperrott indicated that "it is created by an injector which doesn't belong to a specific NgModule so it isn't torn down".

[crisbeto]

It's because TestBed doesn't destroy providedIn: 'root' providers between tests. We have a workaround for it in our own tests: https://github.com/angular/components/blob/master/src/material/dialog/dialog.spec.ts#L86. We're also currently discussing how to handle this.

[crisbeto]

@jbeder have you tried using the new teardown flag in TestBed? We have a test in the Framework repo that verifies something similar to what you're describing. The new flag should be exposed in Catalyst as well.

[jbeder]

Yep, Joey had mentioned that to me, which is why I had pinged him when it didn't work.

…

On Thu, Aug 19, 2021 at 1:55 PM Kristiyan Kostadinov < ***@***.***> wrote: @jbeder <https://github.com/jbeder> have you tried using the new teardown flag in TestBed? We have a test in the Framework repo <https://github.com/angular/angular/blob/master/packages/core/test/test_bed_spec.ts#L1363> that verifies something similar to what you're describing. The new flag should be exposed in Catalyst as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23385 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAICUBQ3WHJZ3QLQKSJ73TDT5VHQ7ANCNFSM5CISGSHQ> .

[crisbeto]

I spent some more time trying to reproduce it, but the only way I could get it to happen was to turn off the teardown flag. Maybe the teardown flag isn't being passed through Catalyst correctly? Have you tried writing the same test using TestBed directly?

[jbeder]

If you'd like a reproducible example, see CL/392502204.

[mmalerba]

@jbeder I think the issue may be tests from other files that do not set the new flag and wind up getting bundled in with the tests in the file you edited. I changed the describe blocks in your CL to describe.only and it passed.

I think there still might be some kind of bug here though, because it shouldn't matter if other describe blocks set different teardown settings. To solve your immediate problem though, you might try just making sure that all of the describe blocks in that compilation unit set the new teardown flag

[jbeder]

@mmalerba First, there is indeed one test in a different file that doesn't set the teardown flag, and I set it to true, and sure enough, the test passes. (The strange thing is that it's a nearly trivial test - it doesn't do anything other than instantiate a very simple injectible class, which is why I didn't add the teardown flag to it.)

But second, I'm confused by your analysis - won't describe.only not even run the other tests (which are the ones failing)? The particular interaction here is that the tests in that one file pass, but when they happen to be executed before tests in a different file, those tests don't pass.

[mmalerba]

Ah I didn't know that. I thought the tests in the CL were the only ones you were concerned with, so I just used describe.only as a way of ensuring that only tests with the flag set were being run. The reason I suspected this is that Catalyst apparently works differently than TestBed externally in that it calls initTestEnvironment before every test (thanks to @crisbeto for pointing this out). I thought it was possible that it wasn't actually updating the behavior with the new flag settings on subsequent calls, so I wanted to make sure all the tests were specifying the flag.

Obviously if that is what's happening, it's still a bug of some sort, but you could at least work around it by making sure that all of your tests have the flag

[jbeder]

SG. Should I keep this bug open, or file a different one about the flag getting overwritten in Catalyst?

…

On Mon, Aug 23, 2021 at 5:39 PM mmalerba ***@***.***> wrote: Ah I didn't know that. I thought the tests in the CL were the only ones you were concerned with, so I just used describe.only as a way of ensuring that only tests with the flag set were being run. The reason I suspected this is that Catalyst apparently works differently than TestBed externally in that it calls initTestEnvironment before every test. I thought it was possible that it wasn't actually updating the behavior with the new flag settings on subsequent calls, so I wanted to make sure all the tests were specifying the flag. Obviously if that is what's happening, it's still a bug of some sort, but you could at least work around it by making sure that all of your tests have the flag — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23385 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAICUBS22H5VCSLGNV7ELDDT6LE35ANCNFSM5CISGSHQ> .

[jbeder]

Also, related: is the plan for this flag to be automatically set for all tests? (If not, why not?)

[crisbeto]

It'll be turned on by default in v13, but we'll have an automated migration that opts existing users out of it explicitly. We can't turn it on by default, because there's a possibility of breaking existing tests. E.g. when we enabled it on our own repo, it revealed some test failures that took a while to track down.