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

feat: move unit tests from Circleci to Github actions #25570

Merged
merged 26 commits into from
Jul 5, 2024

Conversation

itsyoboieltr
Copy link
Contributor

@itsyoboieltr itsyoboieltr commented Jun 27, 2024

Description

Open in GitHub Codespaces

This PR moves unit tests from Circleci to Github actions. This is needed to pass the Jest export to SonarCloud in order finalize the SonarCloud integration. The reason why we want to use Github action rather than Circleci is because Sonarcloud integration happens in a Github action and we cannot pass data (i.e. the artifact) from Circleci to Github action.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2449

Manual testing steps

  1. Seeing unit tests pass in GitHub actions instead of Circleci

Screenshots/Recordings

Not applicable

Pre-merge author checklist

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.

Copy link
Contributor

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.

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.77%. Comparing base (56c9dc2) to head (d1be009).
Report is 14 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25570      +/-   ##
===========================================
+ Coverage    69.67%   69.77%   +0.09%     
===========================================
  Files         1366     1376      +10     
  Lines        48231    48388     +157     
  Branches     13304    13346      +42     
===========================================
+ Hits         33604    33758     +154     
- Misses       14627    14630       +3     

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

@metamaskbot
Copy link
Collaborator

Builds ready [a399875]
Page Load Metrics (46 ± 3 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint66978084
domContentLoaded8171021
load38644673
domInteractive8171021
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [ce18831]
Page Load Metrics (225 ± 246 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7111489126
domContentLoaded9361263
load431778225512246
domInteractive9361263
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@itsyoboieltr itsyoboieltr marked this pull request as ready for review July 1, 2024 16:00
@itsyoboieltr itsyoboieltr requested review from kumavis and a team as code owners July 1, 2024 16:00
@metamaskbot
Copy link
Collaborator

Builds ready [9711df5]
Page Load Metrics (70 ± 11 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint691451002110
domContentLoaded95625136
load43118702211
domInteractive95625136
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@davidmurdoch davidmurdoch left a comment

Choose a reason for hiding this comment

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

just a partial review with some questions before I look more closely.

.circleci/config.yml Outdated Show resolved Hide resolved
.github/workflows/run-unit-tests.yml Outdated Show resolved Hide resolved
.github/workflows/run-unit-tests.yml Outdated Show resolved Hide resolved
.github/workflows/run-unit-tests.yml Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [6f6fbf6]
Page Load Metrics (133 ± 152 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint62206963115
domContentLoaded95523126
load391508133316152
domInteractive95523126
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [15797c2]
Page Load Metrics (356 ± 334 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint683051146431
domContentLoaded998282110
load392237356696334
domInteractive998282110
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@davidmurdoch davidmurdoch left a comment

Choose a reason for hiding this comment

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

As discussed in slack, let's follow up this PR with an investigation into making it go faster (to match the speeds at which Circleci ran the same jobs).

But in case it takes a while to do that work: can you add a new GitHub Issue and a comment in this file linking to the issue, explaining that we should look into making it go faster?

I just don't want people copying this work in the (near) future, not noticing the slow down, and suddenly our tests take hours to pass CI. Hopefully a comment in here will help prevent that.

relevant:
image

@HowardBraham
Copy link
Contributor

As discussed in slack, let's follow up this PR with an investigation into making it go faster (to match the speeds at which Circleci ran the same jobs).

There are 2 reasons it's slower than CircleCI:

  1. It's not using the yarn cache from CircleCI, so it has to redo the full work of CircleCI's prep-deps
  2. CircleCI is running with parallelism 8 (which is honestly too high)

@itsyoboieltr
Copy link
Contributor Author

itsyoboieltr commented Jul 4, 2024

There are 2 reasons it's slower than CircleCI:

  1. It's not using the yarn cache from CircleCI, so it has to redo the full work of CircleCI's prep-deps
  2. CircleCI is running with parallelism 8 (which is honestly too high)

@HowardBraham Do you think the first point is really a reason for the slow down? I do not necessarily think so, since it is using the yarn cache from GitHub, meaning it does not need to redo the full work.

The second point about parallelism is true, we do need a fix for that.

Copy link

sonarcloud bot commented Jul 4, 2024

Copy link
Contributor

@davidmurdoch davidmurdoch left a comment

Choose a reason for hiding this comment

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

⭐⭐⭐⭐⭐
Five stars!
Would review again.

@metamaskbot
Copy link
Collaborator

Builds ready [d1be009]
Page Load Metrics (177 ± 216 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint76135107189
domContentLoaded96131178
load412134177449216
domInteractive96130178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham
Copy link
Contributor

@itsyoboieltr In the prep-deps step, CircleCI does a persist_to_workspace on the node_modules folder. So in future steps, it does not run yarn install at all, it just does attach_workspace, which brings back the node_modules. This step tends to take 14 seconds.

@itsyoboieltr itsyoboieltr merged commit 2050886 into develop Jul 5, 2024
71 checks passed
@itsyoboieltr itsyoboieltr deleted the move-unit-tests-to-github-actions branch July 5, 2024 14:26
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2024
@metamaskbot metamaskbot added the release-12.2.0 Issue or pull request that will be included in release 12.2.0 label Jul 5, 2024
name: Run unit tests

on:
pull_request:
Copy link
Member

Choose a reason for hiding this comment

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

We need to add a trigger for develop and master here as well. These long-running branches will not have PRs associated with them, but it's very important that we know that tests are passing on them.

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

Successfully merging this pull request may close these issues.

6 participants