Skip to content

Replace shared mocks in incoming transaction controller tests#9754

Merged
Gudahtt merged 1 commit intodevelopfrom
incoming-transaction-tests-replace-shared-mocks
Oct 29, 2020
Merged

Replace shared mocks in incoming transaction controller tests#9754
Gudahtt merged 1 commit intodevelopfrom
incoming-transaction-tests-replace-shared-mocks

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Oct 29, 2020

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.

@Gudahtt Gudahtt marked this pull request as ready for review October 29, 2020 05:04
@Gudahtt Gudahtt requested a review from a team as a code owner October 29, 2020 05:04
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [d55dfd8]
Page Load Metrics (482 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31125552814
domContentLoaded31181648014770
load31381848214770
domInteractive31181648014770

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.
@Gudahtt Gudahtt force-pushed the incoming-transaction-tests-replace-shared-mocks branch from d55dfd8 to a0a2dcb Compare October 29, 2020 06:21
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a0a2dcb]
Page Load Metrics (446 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318039115
domContentLoaded30668844513967
load30868944613967
domInteractive30668844413967

Copy link
Copy Markdown
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Gudahtt Gudahtt merged commit d99d859 into develop Oct 29, 2020
@Gudahtt Gudahtt deleted the incoming-transaction-tests-replace-shared-mocks branch October 29, 2020 15:16
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants