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

test: Simplify Jest config and expand coverage #25013

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jun 3, 2024

Description

Our Mocha and Jest configuration has been messy because we're in the middle of a migration from Mocha to Jest. Each file had to be explicitly included in the Jest configuration or ignored in the Mocha configuration. This was inconvenient and error-prone, and resulted in some tests not being run at all.

Both test configurations have been updated to use a shared list of Mocha test files. There are only a few of these files left, and this list should only get shorter as we migrate more tests to Jest. No further configuration changes will be needed to add Jest tests.

We have now finished migrating unit tests from Mocha to Jest, so this PR now only affects the Jest configuration.

Note that the ESLint configuration has not been updated to use these simpler globs to determine which tests use Mocha and which use Jest. I tried doing that in this PR initially but it raised too many lint errors, so that will come in a later PR. In the meantime, we may still need to update .eslintrc.js when adding a new test of either type.

Open in GitHub Codespaces

Related issues

N/A

Manual testing steps

Run yarn test:unit and verify that it runs correctly (no errors, no missing tests)

Screenshots/Recordings

N/A

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@Gudahtt

This comment was marked as resolved.

@Gudahtt Gudahtt force-pushed the fix-test-config branch 4 times, most recently from 443d685 to 0c04e01 Compare June 6, 2024 13:15
@Gudahtt Gudahtt marked this pull request as ready for review June 6, 2024 13:15
@Gudahtt Gudahtt requested a review from a team as a code owner June 6, 2024 13:15
@Gudahtt Gudahtt marked this pull request as draft June 6, 2024 14:02
@Gudahtt

This comment was marked as resolved.

@Gudahtt Gudahtt force-pushed the fix-test-config branch 2 times, most recently from d6bc055 to f9029b7 Compare June 14, 2024 23:23
@Gudahtt Gudahtt marked this pull request as ready for review June 14, 2024 23:25
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.62%. Comparing base (64b8db4) to head (dda8b8c).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25013      +/-   ##
===========================================
- Coverage    70.44%   69.62%   -0.83%     
===========================================
  Files         1274     1344      +70     
  Lines        44080    47795    +3715     
  Branches     12453    13190     +737     
===========================================
+ Hits         31051    33273    +2222     
- Misses       13029    14522    +1493     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [6d47993]
Page Load Metrics (165 ± 224 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6816188209
domContentLoaded8271352
load392198165467224
domInteractive8271352
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt force-pushed the fix-test-config branch 2 times, most recently from 7ac6963 to 5c1b2f5 Compare June 20, 2024 16:44
@metamaskbot
Copy link
Collaborator

Builds ready [5c1b2f5]
Page Load Metrics (124 ± 148 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint661028494
domContentLoaded9131111
load421463124307148
domInteractive9131111
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jun 20, 2024
@Gudahtt
Copy link
Member Author

Gudahtt commented Jun 24, 2024

Rebased to resolve conflicts, and the Mocha test coverage list has been removed because it was now empty.

@metamaskbot
Copy link
Collaborator

Builds ready [154d349]
Page Load Metrics (49 ± 3 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint67897963
domContentLoaded8131111
load41624963
domInteractive8131111
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Our Mocha and Jest configuration has been messy because we're in the
middle of a migration from Mocha to Jest. Each file had to be
explicitly included in the Jest configuration or ignored in the Mocha
configuration. This was inconvenient and error-prone, and resulted in
some tests not being run at all.

Both test configurations have been updated to use a shared list of
Mocha test files. There are only a few of these files left, and this
list should only get shorter as we migrate more tests to Jest. No
further configuration changes will be needed to add Jest tests.
@Gudahtt Gudahtt marked this pull request as draft June 24, 2024 18:32
@Gudahtt
Copy link
Member Author

Gudahtt commented Jun 24, 2024

More conflicts to resolve. Moving into draft (CI passes but it shouldn't, this is reducing test coverage).

@Gudahtt Gudahtt changed the title test: Fix test config test: Simplify Jest config and expand coverage Jun 24, 2024
@Gudahtt Gudahtt marked this pull request as ready for review June 24, 2024 18:47
'<rootDir>/app/scripts/migrations/*.ts',
'!<rootDir>/app/scripts/migrations/*.test.(js|ts)',
'<rootDir>/app/scripts/platforms/*.js',
'<rootDir>/app/scripts/**/*.(js|ts|tsx)',
Copy link
Contributor

Choose a reason for hiding this comment

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

wow, nice

@Gudahtt Gudahtt merged commit 18ebe12 into develop Jun 24, 2024
78 of 79 checks passed
@Gudahtt Gudahtt deleted the fix-test-config branch June 24, 2024 19:18
@github-actions github-actions bot locked and limited conversation to collaborators Jun 24, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jun 24, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 24, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [dda8b8c]
Page Load Metrics (45 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint5910376115
domContentLoaded8191021
load38694584
domInteractive8191021
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants