Replace shared mocks in incoming transaction controller tests#9754
Merged
Replace shared mocks in incoming transaction controller tests#9754
Conversation
Collaborator
Builds ready [d55dfd8]
Page Load Metrics (482 ± 70 ms)
|
The shared mocks used previously in the incoming transaction controller tests have been replaced with functions that can generate a new mock for each test. We should avoid ever sharing mocks between tests. It's quite easy for a mock to get accidentally mutated or not correctly "reset" for the next test, leading to test inter-dependencies and misleading results. In particular, it is unsafe to share a `sinon` fake (e.g. a spy or stub) because they can't be fully reset between tests. Or at least it's difficult to reset them property, and it can't be done while also following their recommendations for preventing memory leaks. The spy API and all related state can be reset with `resetHistory`, which can be called between each test. However `sinon` also recommends calling `restore` after each test, and this will cause `sinon` to drop its internal reference to the fake object, meaning any subsequent call to `resetHistory` would fail. This is intentional, as it's required to prevent memory from building up during the test run, but it also means that sharing `sinon` fakes is particularly difficult to do safely. Instead we should never share mocks in the first place, which has other benefits anyway. This was discovered while writing tests for #9583. I mistakenly believed that `sinon.restore()` would reset the spy state, and this was responsible for many hours of debugging test failures.
d55dfd8 to
a0a2dcb
Compare
Collaborator
Builds ready [a0a2dcb]
Page Load Metrics (446 ± 67 ms)
|
rekmarks
reviewed
Oct 29, 2020
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The shared mocks used previously in the incoming transaction controller tests have been replaced with functions that can generate a new mock for each test.
We should avoid ever sharing mocks between tests. It's quite easy for a mock to get accidentally mutated or not correctly "reset" for the next test, leading to test inter-dependencies and misleading results.
In particular, it is unsafe to share a
sinonfake (e.g. a spy or stub) because they can't be fully reset between tests. Or at least it'sdifficult to reset them property, and it can't be done while also following their recommendations for preventing memory leaks.
The spy API and all related state can be reset with
resetHistory, which can be called between each test. Howeversinonalso recommends callingrestoreafter each test, and this will causesinonto drop its internal reference to the fake object, meaning any subsequent call toresetHistorywould fail. This is intentional, as it's required to prevent memory from building up during the test run, but it also means that sharingsinonfakes is particularly difficult to do safely.Instead we should never share mocks in the first place, which has other benefits anyway.
This was discovered while writing tests for #9583. I mistakenly believed that
sinon.restore()would reset the spy state, and this was responsible for many hours of debugging test failures.