-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(store): remove afterEach hook in mock store #3245
Conversation
don't know why ci/circleci: schematics-core-check failed on CircleCI |
Preview docs changes for 9621596 at https://previews.ngrx.io/pr3245-9621596b/ |
const mockStore: MockStore | undefined = TestBed.inject(MockStore); | ||
if (mockStore) { | ||
mockStore.resetSelectors(); | ||
const TestBed = getTestBed() as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here if why this is needed, so we'll have some context later when we can potentially remove it. Maybe just a link to the issue #
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, not sure if my english is good enough, but I tried ...
07c9824
to
0c76508
Compare
710c6f4
to
52a745a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one question, the rest of the PR seems good to me.
@timdeschryver maybe we should remove or update the following text in the doc (https://ngrx.io/guide/store/testing) :
|
@cedricduffournet good catch! Feel free to do that if you want. |
@timdeschryver I updated the doc and added a test, maybe not necessary, to show how reset selectors in afterEach hook. Let me know if you prefer that I remove it |
@@ -75,7 +75,7 @@ In this example based on the [walkthrough](guide/store/walkthrough), we mock the | |||
|
|||
<div class="alert is-helpful"> | |||
|
|||
**Note:** `MockStore` will reset all of the mocked selectors after each test (in the `afterEach()` hook) by calling the `MockStore.resetSelectors()` method. | |||
**Note:** `MockStore` will **not** reset mocked selectors after each test. You can reset selectors by calling the `MockStore.resetSelectors()` method in `afterEach()` hook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this alert, and instead, show how you can reset the store manually after each test.
About the re-added tests. I think we're already testing that resetSelector
does what it's supposed to do, so these are redundant and can be removed.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that sounds good to me. I was not sure about this test.
I'll add a code example in the doc
2e76aa4
to
28e2453
Compare
28e2453
to
5587f5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @cedricduffournet .
I left a few comments to make the example more compact.
projects/ngrx.io/content/examples/store-walkthrough/src/app/tests/app.component.1.spec.ts
Outdated
Show resolved
Hide resolved
projects/ngrx.io/content/examples/store-walkthrough/src/app/tests/app.component.1.spec.ts
Outdated
Show resolved
Hide resolved
projects/ngrx.io/content/examples/store-walkthrough/src/app/tests/app.component.1.spec.ts
Outdated
Show resolved
Hide resolved
projects/ngrx.io/content/examples/store-walkthrough/src/app/tests/app.component.1.spec.ts
Show resolved
Hide resolved
projects/ngrx.io/content/examples/store-walkthrough/src/app/tests/app.component.1.spec.ts
Outdated
Show resolved
Hide resolved
35f2318
to
639c66a
Compare
639c66a
to
9621596
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience and thoroughness @cedricduffournet .
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Closes #3243
What is the new behavior?
Does this PR introduce a breaking change?
MockStore
will not reset all of the mocked selectors after each test. User will need to call theMockStore.resetSelectors()
method manually in aafterEach
hook of adescribe
blockOther information