Selectors in MockStore not reset even with destroyAfterEach

sbarfurth commented 4 months ago

Which @ngrx/* package(s) are the source of the bug?

store

Minimal reproduction of the bug/regression with instructions

Reproduction in sbarfurth/selector-reset-repro.

Run ng test to reproduce.

Expected behavior

All tests pass.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

NgRx: v18.0.1 Angular: v18.0.7 Node: v20.15.0 Browser: Chrome v126 OS: macOS Sonoma 14.5

Other information

The automagic reset of the MockStore was removed with v13 as a breaking change. The migration guide states that this is a non-issue for tests using destroyAfterEach: https://ngrx.io/guide/migration/v13#testing-reset-mock-store

The overrideSelector method of MockStore sets the value on the memoized selector itself using setResult when not using a string. This values will leak between tests regardless of whether the TestBed is torn down or not since the memoized selector is only imported once per test suite.

import {someSelector} from './selectors';

// Test 1
mockStore.overrideSelector(someSelector, 10);
// calls someSelector.setResult(10)

// Reset TestBed
testBed. resetTestEnvironment();
// The imported memoized selector is not touched by this.

// Test 2
mockStore.select(someSelector);
// simply retrieves the result in selector (10)

This can be remedied by manually calling resetSelectors on the MockStore since that calls release and clearResult for every selector.

It seems the migration guide is incorrect and that this implication of overrideSelector is undocumented.

I would be willing to submit a PR to fix this issue

[ ] Yes
[ ] No

timdeschryver commented 1 month ago

@sbarfurth maybe that the v13 migration guide isn't really clear/specific about this.

Destroying the TestBed has little to do with resetting the cached value of a selector. To reset the selectors you must use resetSelectors.

getTestBed().inject(MockStore, null).resetSelectors();

The problem occurs when relying on Angular to teardown the Store. While doing so, resetSelectors loses its context and cannot reset previously set selectors.

sbarfurth commented 1 month ago

Yes, I realize this. However, the migration guide suggests that the behavior is somehow different or a non-issue when using destroyAfterEach. It seems to me that simply isn't true? As far as I can tell this has no bearing on whether or not you need to reset selectors. I'm only asking if it wouldn't make sense to update the migration guide since I think the current version is unclear at best. It might even be incorrect.

ngrx / platform