Skip to content
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

Merged
merged 8 commits into from
Dec 7, 2021

Conversation

cedricduffournet
Copy link
Contributor

@cedricduffournet cedricduffournet commented Nov 19, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #3243

What is the new behavior?

Does this PR introduce a breaking change?

[x] Yes
[] No

MockStore will not reset all of the mocked selectors after each test. User will need to call the MockStore.resetSelectors() method manually in a afterEach hook of a describe block

Other information

@cedricduffournet
Copy link
Contributor Author

don't know why ci/circleci: schematics-core-check failed on CircleCI

@ngrxbot
Copy link
Collaborator

ngrxbot commented Nov 19, 2021

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;
Copy link
Member

@brandonroberts brandonroberts Nov 19, 2021

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 #

Copy link
Contributor Author

@cedricduffournet cedricduffournet Nov 19, 2021

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

@cedricduffournet cedricduffournet changed the title fix(store): do not reset selectors if destroyAfterEach is enabled fix(store): remove afterEach hook in mock store Dec 2, 2021
Copy link
Member

@timdeschryver timdeschryver left a 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.

modules/store/testing/spec/mock_store.spec.ts Show resolved Hide resolved
@cedricduffournet
Copy link
Contributor Author

@timdeschryver maybe we should remove or update the following text in the doc (https://ngrx.io/guide/store/testing) :

Note: MockStore will reset all of the mocked selectors after each test (in the afterEach() hook) by calling the MockStore.resetSelectors() method.

@timdeschryver
Copy link
Member

timdeschryver commented Dec 2, 2021

@cedricduffournet good catch! Feel free to do that if you want.

@cedricduffournet
Copy link
Contributor Author

@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.
Copy link
Member

@timdeschryver timdeschryver Dec 3, 2021

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?

Copy link
Contributor Author

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

@cedricduffournet cedricduffournet force-pushed the fix-jest-test branch 2 times, most recently from 2e76aa4 to 28e2453 Compare December 6, 2021 16:50
Copy link
Member

@timdeschryver timdeschryver left a 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.

Copy link
Member

@timdeschryver timdeschryver left a 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 .

@brandonroberts brandonroberts merged commit 0640085 into ngrx:master Dec 7, 2021
@cedricduffournet cedricduffournet deleted the fix-jest-test branch December 7, 2021 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when executing test with jest and jest-circus as testRunner
4 participants