Chore: fix runaway test issue #6460
Conversation
| }) | ||
| export class LayoutContainer implements OnDestroy { | ||
| readonly runsTableFullScreen$ = this.store.select(getRunsTableFullScreen); | ||
| private readonly ngUnsubscribe = new Subject<void>(); |
There was a problem hiding this comment.
I'm ok with this fix, but it feels to some extent like it's just working around the issue, rather than addressing it, so I have some thoughts based on what I've gathered so far, although I recognize what I say might be completely wrong/inaccurate. A proper fix can be addressed separately, if we can indeed identify the issue, but I think doing it would simplify writing these components without worrying about "unsubscribing" from the NGRX selectors.
My best guess of why we might have these issues is the way we use the TestBed across different tests (and between setup and the test/spec execution) as a "global" (by calling "class methods" on it, e.g. TestBed.configureTestingModule() ), and this might be causing issues when running multiple tests concurrently. Particularly, the fact that we call the "configure" method in the beforeEach test, but often create the fixture (or maybe even do other things with the test bed?) within individual tests, might be causing some injection of dependencies to be crossed between tests, or something like this.
The more common pattern I've seen is where the configure method and the "fixture creation" (along with other references to the testbed, e.g. for injecting values) are together within the same beforeEach block, like in the NGRX examples:
https://v8.ngrx.io/guide/store/testing#using-mock-selectors
I think that should be the safest thing to do.
If isolating the calls to TestBed in a single block is not feasible or desirable for some reason, we might be able to instead store each TestBed instance in a variable that is accessible from tests, which might avoid that "shared global" issue (assuming that diagnostic is somewhat right, but again, I'm not really sure).
Let me know what you think.
There was a problem hiding this comment.
Just noting here for the record: we tested this while we were talking in person. There's some chance that we didn't do something right, but as best as I can tell, we did.
We also realized there were other tests that were changing this value, but not resetting the selectors, and resetting the selectors in those other tests seems to mostly get rid of the issue (in the sense that other tests would now be using the same value as the default, but maybe they're still accessing the same selector instance?). It's still unclear whether and why the selector from one "test bed" gets used by other tests.
2 participants
Motivation for features / changes
In #6458 I fixed a couple of a tests which should have been failing but were sporadically passing. In this PR I believe I have fixed the underlying cause of the flakiness.
Thanks #6266 for pointing the way.
This broke nightly last night
https://github.com/tensorflow/tensorboard/actions/runs/5388986210/jobs/9782358616