Always vi.restoreAllMocks() before each test #5788
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mstrasinskis
approved these changes
Nov 15, 2024
1 task
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 18, 2024
# Motivation Since #5788, `vi.restoreAllMocks()` is automatically called before each test. # Changes 1. Remove redundant calls to `vi.restoreAllMocks()` from individual tests. # Tests Still pass. # Todos - [ ] Add entry to changelog (if necessary). not necessary
1 task
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 21, 2024
# Motivation Since #5788 we automatically always call `vi.restoreAllMocks` before every test. `vi.clearAllMocks()` and `vi.resetAllMocks()` do strictly less so using them in `beforeEach` is redundant. # Changes Remove all calls to `vi.clearAllMocks()` and `vi.resetAllMocks()` from `beforeEach` in tests. # Tests Still pass # Todos - [ ] Add entry to changelog (if necessary). not necessary
1 task
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 25, 2024
# Motivation In line with #5788, this PR ensures that all globals are restored before each test. # Changes - Moves `unstubAllGlobals` to `vitest.setup.ts`. # Tests All tests pass. # Todos - [ ] Add entry to changelog (if necessary). Not necessary.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 8, 2025
# Motivation Our unit tests have always been quite brittle because we need to remember to clean up all global state between each test. Often tests fail when run in a different order because one test accidentally depends on state left behind by other tests. This has improved a lot now that we always [reset all stores](#5724) and [restore all mocks](#5788), but there are still other cleanups that need to be performed in tests. This PR proposes a generic way of registering a cleanup in the file that defines the thing that needs to be cleaned up, instead of in every test. As a proof of concept, it is then used to automatically call `resetMockedConstants();` in every test that (transitively) imports `$tests/utils/mockable-constants.test-utils.ts`. # Changes 1. Create an empty `registerCleanupForTesting` function which should be used to register cleanup functions. 2. In `frontend/vitest.setup.ts` mock `frontend/src/lib/utils/test-support.utils.ts` such that the registered cleanup functions are called before every test. # Tests Tests only # Todos - [ ] Add entry to changelog (if necessary). not necessary
1 task
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 8, 2025
# Motivation
These 2 tests in `SnsProposalDetail.spec.ts`:
```
"should navigate to the proposal from the previous Sns"
"should navigate to the proposal from the next Sns"
```
are currently logging errors like this:
```
{
certified: false,
strategy: 'query_and_update',
error: t [Error]: No proposal for given proposalId 19
at Object.queryProposal (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/tests/fakes/sns-governance-api.fake.ts:327:11)
at fn (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/tests/utils/module.test-utils.ts:158:33)
at wrapMaybePaused (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/tests/utils/module.test-utils.ts:140:20)
at Module.pausableFunctions.<computed> (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/tests/utils/module.test-utils.ts:157:14)
at Module.mockCall (file:///Users/dskloet/dev/nns-dapp/tree4/frontend/node_modules/@vitest/spy/dist/index.js:61:17)
at Module.queryProposal (file:///Users/dskloet/dev/nns-dapp/tree4/frontend/node_modules/tinyspy/dist/index.js:45:80)
at request (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/lib/services/public/sns-proposals.services.ts:209:7)
at queryOrUpdate (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/lib/services/utils.services.ts:85:5)
at Module.queryAndUpdate (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/lib/services/utils.services.ts:105:17)
at Module.getSnsProposalById (/Users/dskloet/dev/nns-dapp/tree4/frontend/src/lib/services/public/sns-proposals.services.ts:206:10),
identity: {
getPrincipal: [Function: getPrincipal],
transformRequest: [Function: transformRequest]
}
}
```
This started happening after
#5788
Before that PR, the tests were relying on `console.error` being mocked
in the `beforeEach` of a different `describe` block but after that PR
all mocks are always restored before each test so the mocking of
`console.error` was being reverted.
However logging during tests is not allowed and normally results in test
failure. However these test passed before the logging happened so the
logging could happen without the tests failing.
And in general mocking `console.error` in `beforeEach` is dangerous
because it can hide problems in other tests.
It's best to only mock `console.error` in tests that expect logging and
always expect the specific messages being logged so no other errors are
hidden.
# Changes
1. Stop mocking `console.error` in `beforeEach` in
`SnsProposalDetail.spec.ts`.
2. Mock `console.error` in specific tests where errors are logged and
expect what is being logged.
3. In `"show neurons that can vote"`, add a missing proposal that the
test expects. It was logging errors for this that were ignored. The test
does not seem to intend the proposal to be missing.
4. Add `await runResolvedPromises();` in `afterEach` in
`failTestsThatLogToConsole` in order to prevent tests from passing
before they log some errors.
5. In `queryProposal` in the fake SNS API, take into account that there
may not be any proposals for an identity+SNS combination and allow to
throw the intended error instead of `undefined.proposals` not existing.
This happened in one of the tests making the logged error message even
more confusing.
# Tests
Tests only
# Todos
- [ ] Add entry to changelog (if necessary).
not necessary
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
To avoid unpredictable behavior, each test should start with a clean slate.
One component of that is restoring all mocks before each test.
This PR makes sure all mocks are restored before each test.
I thought all tests were already prepared in earlier PRs but some were apparently missed on my first pass.
This PR fixes all the remaining tests as well.
A follow-up PR will remove now unnecessary calls to
vi.restroreAllMock()from individual tests.Changes
vi.restoreAllMocks()inbeforeEachinvitest.setup.ts.Tests
All tests pass.
Todos
not necessary