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

Refactor: Construct the network controller within each test #17199

Merged
merged 6 commits into from
Jan 17, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jan 13, 2023

The network controller is now constructed within each network controller unit test, rather than in the beforeEach. This allows us to customize the constructor options in each test, which some planned future tests will require.

The controller is constructed with a helper function that also handles calling destroy after each test, even if the test failed. This helps to prevent tests from affecting each other.

This relates to #16962

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@Gudahtt
Copy link
Member Author

Gudahtt commented Jan 13, 2023

This depends upon #17126

@Gudahtt Gudahtt force-pushed the network-controller-construct-per-test branch from 449b30d to 1c6d8a7 Compare January 13, 2023 18:04
@metamaskbot
Copy link
Collaborator

Builds ready [1c6d8a7]
Page Load Metrics (1186 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint87152103157
domContentLoaded1057136911708440
load1059137011867838
domInteractive1057136911708440
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

@codecov-commenter
Copy link

Codecov Report

Merging #17199 (1c6d8a7) into replace-network-controller-test-internal-mocks (dc39e2e) will not change coverage.
The diff coverage is n/a.

@@                               Coverage Diff                               @@
##           replace-network-controller-test-internal-mocks   #17199   +/-   ##
===============================================================================
  Coverage                                           59.55%   59.55%           
===============================================================================
  Files                                                 936      936           
  Lines                                               35959    35959           
  Branches                                             9213     9213           
===============================================================================
  Hits                                                21414    21414           
  Misses                                              14545    14545           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

A few suggestions.

Base automatically changed from replace-network-controller-test-internal-mocks to develop January 17, 2023 18:09
The network controller is now constructed within each network
controller unit test, rather than in the `beforeEach`. This allows us
to customize the constructor options in each test, which some planned
future tests will require.

The controller is constructed with a helper function that also handles
calling `destroy` after each test, even if the test failed. This helps
to prevent tests from affecting each other.
@Gudahtt Gudahtt force-pushed the network-controller-construct-per-test branch from 1c6d8a7 to bd6cdd4 Compare January 17, 2023 18:12
@Gudahtt Gudahtt marked this pull request as ready for review January 17, 2023 18:16
@Gudahtt Gudahtt requested a review from a team as a code owner January 17, 2023 18:16
@Gudahtt Gudahtt requested a review from mcmire January 17, 2023 18:16
Copy link
Contributor

@BelfordZ BelfordZ left a comment

Choose a reason for hiding this comment

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

looks great!

@metamaskbot
Copy link
Collaborator

Builds ready [714402c]
Page Load Metrics (1515 ± 94 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint104146129136
domContentLoaded11931947150520096
load12601948151519694
domInteractive11931947150520096
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

@Gudahtt Gudahtt merged commit 7d77554 into develop Jan 17, 2023
@Gudahtt Gudahtt deleted the network-controller-construct-per-test branch January 17, 2023 23:28
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2023
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.

5 participants