platform icon indicating copy to clipboard operation
platform copied to clipboard

Selectors in MockStore not reset even with destroyAfterEach

Open sbarfurth opened this issue 1 year ago • 2 comments

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 value 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

sbarfurth avatar Jul 08 '24 14:07 sbarfurth

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

timdeschryver avatar Sep 12 '24 11:09 timdeschryver

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.

sbarfurth avatar Sep 17 '24 06:09 sbarfurth