Skip to content

Move ganache server from setup file to individual test suites#9703

Merged
tmashuang merged 1 commit intodevelopfrom
mv-ganache-setup
Feb 4, 2021
Merged

Move ganache server from setup file to individual test suites#9703
tmashuang merged 1 commit intodevelopfrom
mv-ganache-setup

Conversation

@tmashuang
Copy link
Copy Markdown
Contributor

@tmashuang tmashuang commented Oct 23, 2020 •
edited
Loading

Jest setup files run on each individual test file/suite. This moves the Ganache out of the test setup files and into the tests that Ganache is used. One issue is at the end of the unit test output. there is an error from Ganache that I am unfamiliar with how to handle.

Error output screenshot
Strikethough no longer valid now that #10331 has been merged.

@tmashuang tmashuang requested a review from a team as a code owner October 23, 2020 19:55
@tmashuang tmashuang requested a review from whymarrh October 23, 2020 19:55
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 23, 2020

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Gudahtt
Gudahtt reviewed Oct 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

This is a fantastic idea 👍 Thanks!

Curious error 🤔 I might spend some time looking into it soon.

@tmashuang tmashuang marked this pull request as draft October 23, 2020 20:39
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Oct 24, 2020

I have suspected some of the other flaky unit test behaviour we've seen was caused by tests running after mocha thinks they're finished. Either that or the code under test keeps running. I bet this is caused by the same thing.

My plan was going to be to remove the --exit from the mocha command in yarn test:unit and try each test one module at a time to see if it exits properly. The test setup might have to be adjusted to exit properly too; not sure.

@danjm danjm added this to the v8.1.next? milestone Nov 2, 2020
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Nov 7, 2020

This error appears to be caused by test/unit/ui/app/actions.spec.js - it appears even when only that file is run.

I suspect that the code-under-test in this test suite keeps running long after the tests end, and it blows up when ganache stops.

These tests are not great 😬. Ultimately we should be using mocks for each of these actions, instead of a real instance of the MetaMask background controller. We might have to go through these action tests one-by-one and replace the use of this controller with mocks.

@Gudahtt Gudahtt closed this Nov 7, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2020
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Nov 7, 2020

Oops - I clicked "Close and comment" by accident instead of "Comment".

@Gudahtt Gudahtt reopened this Nov 7, 2020
@MetaMask MetaMask unlocked this conversation Nov 7, 2020
@Gudahtt Gudahtt assigned tmashuang and unassigned Gudahtt Jan 26, 2021
@tmashuang
Copy link
Copy Markdown
Contributor Author

tmashuang commented Feb 1, 2021

Dependent on #10321 as per the latest comment.

@tmashuang tmashuang marked this pull request as ready for review February 3, 2021 20:09
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 3, 2021

Builds ready [030a633]
Page Load Metrics (569 ± 13 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43695484
domContentLoaded5296475672713
load5306485692713
domInteractive5286475672713

Gudahtt
Gudahtt previously approved these changes Feb 3, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 4, 2021

Builds ready [a7ea3dd]
Page Load Metrics (579 ± 27 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint44795694
domContentLoaded3806525775627
load3856535795627
domInteractive3796525775627

@tmashuang tmashuang merged commit 418662c into develop Feb 4, 2021
@tmashuang tmashuang deleted the mv-ganache-setup branch February 4, 2021 18:51
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@whymarrh whymarrh Awaiting requested review from whymarrh

Assignees

@tmashuang tmashuang

Labels

None yet

Projects

None yet

Milestone

v8.1.next?

Development

Successfully merging this pull request may close these issues.

4 participants

@tmashuang @Gudahtt @metamaskbot @danjm