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: improved way to trigger mmi e2e tests #27932

Merged
merged 38 commits into from
Oct 25, 2024
Merged

Conversation

albertolive
Copy link
Contributor

@albertolive albertolive commented Oct 17, 2024

Description

Summary

This pull request introduces a conditional check in the CircleCI pipeline to determine whether MMI Playwright tests should be executed based on the presence of the team-mmi label or if a reviewer from the MetaMask/mmi team has been assigned to the pull request. This optimizes the CI workflow by running MMI-specific tests only when necessary.

Previously, the end-to-end tests were only triggered when the team-mmi label was added to a pull request. However, in practice, no one outside of our team was adding this label, which resulted in the end-to-end MMI tests never being executed.

We already have an automation in place to automatically add a reviewer from the MetaMask/mmi team when relevant pull requests are opened. This pull request leverages that automation to ensure that the MMI tests are automatically triggered when a reviewer is assigned, making the process more efficient and reducing manual steps.

Changes Implemented

  1. MMI Trigger Check
  • Added a check-mmi-trigger step that runs a shell script to check if the pull request has either:
    • A team-mmi label.
    • A reviewer from the MetaMask/mmi team assigned.
  • If either condition is met, the MMI-specific tests will be triggered. Otherwise, the workflow halts and skips these tests.
  1. Modified Workflow
  • The prep-build-test-mmi-playwright and test-e2e-mmi-playwright steps are now conditionally executed based on the result of the check-mmi-trigger step.
  • If the MMI trigger is not present, the workflow halts after the check-mmi-trigger step, improving pipeline efficiency by avoiding unnecessary test execution.
  1. Shell Script (check_mmi_trigger.sh)
  • Added a script to check for the presence of the team-mmi label or the MetaMask/mmi reviewer in a pull request.
  • The script sets the run_mmi_tests environment variable to true or false depending on the conditions, which controls whether the MMI tests will proceed.

Benefits

  • Increased efficiency: MMI Playwright tests are run only when necessary, reducing CI resource usage and speeding up the overall workflow.
  • Clearer conditions for MMI tests: The conditions for triggering MMI-specific tests are now based on both labels and reviewer assignments, ensuring greater flexibility and accuracy in when the tests are executed.

Related issues

Fixes:

Manual testing steps

  1. Go to the CircleCI Project Page:
  • Navigate to the project pipeline in CircleCI.
  1. Trigger a Workflow:
  • Manually trigger the workflow by running a new build. You can either rerun a previous build or push a new commit to the branch you’re testing.
  1. Monitor the Workflow:
  • Observe the pipeline’s progress and ensure that the correct jobs (like MMI Playwright tests) are triggered based on your implemented conditions.
  • Check that the check-mmi-trigger step is evaluated correctly and logs the expected conditions met or skipped.
  1. Verify Expected Behavior:
  • If the conditions for MMI tests (label/reviewer) are met, verify that the prep-build-test-mmi-playwright and test-e2e-mmi-playwright steps run successfully.
  • If conditions are not met, ensure the MMI tests are skipped as expected.
  1. Check Logs:
  • Review the logs for each step to confirm that the correct conditions triggered the tests or that they were correctly skipped.

Screenshots/Recordings

Before

image

After

image
image

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.

@github-actions github-actions bot added the team-mmi PRs from the MMI team label Oct 17, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [fee43b2]
Page Load Metrics (2103 ± 212 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint165134222091406195
domContentLoaded159926402011300144
load164536152103441212
domInteractive13236625828
backgroundConnect99198819694
firstReactRender471881032813
getState4115283316
initialActions01000
loadScripts118921171515242116
setupStore1190302311
uiStartup185738892348494237
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [c135693]
Page Load Metrics (2101 ± 169 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25228592013532255
domContentLoaded157927962056331159
load159028562101352169
domInteractive25163603215
backgroundConnect9132373416
firstReactRender433041005225
getState592282412
initialActions00000
loadScripts115322221535288138
setupStore10101392713
uiStartup177532142339422203
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [fb05163]
Page Load Metrics (2115 ± 103 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39127792014430206
domContentLoaded17092521207720096
load177226942115214103
domInteractive2510450199
backgroundConnect8255435325
firstReactRender601991122814
getState6129252914
initialActions01000
loadScripts12451865152516579
setupStore1392402713
uiStartup197634242375317152
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@albertolive albertolive marked this pull request as ready for review October 18, 2024 10:21
shane-t
shane-t previously approved these changes Oct 18, 2024
zone-live
zone-live previously approved these changes Oct 18, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [faf540d]
Page Load Metrics (1911 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29122961735496238
domContentLoaded16362355186218087
load16402364191118589
domInteractive178047189
backgroundConnect9146533718
firstReactRender49285945124
getState5179374120
initialActions01000
loadScripts11591768135113866
setupStore1195362512
uiStartup179527582166257123
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@albertolive
Copy link
Contributor Author

@Gudahtt could you change the ci/circleci: check-pr-tag to be check-mmi-trigger? It is expecting the old one

- run:
name: Check for MMI Team Label or Reviewer
command: |
chmod +x .circleci/scripts/check_mmi_trigger.sh
Copy link
Member

Choose a reason for hiding this comment

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

The file permissions are checked into the repository; you don't need to make it executable every run, you can make it executable once and commit it.

@Gudahtt
Copy link
Member

Gudahtt commented Oct 21, 2024

@albertolive Thanks for the ping. I have removed check-pr-tag as a required check. I'll leave the new one as optional for now as well

Copy link
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.

Mostly looks good! We can remove the chmod +x step, and we still need to better handle PRs from forks

- Update check_mmi_trigger.sh to skip tests for forks without failing
- Remove redundant chmod from CircleCI config
@albertolive albertolive dismissed stale reviews from zone-live and shane-t via 8844e35 October 22, 2024 07:49
@albertolive
Copy link
Contributor Author

@Gudahtt Thank you for your comments. I’ve updated the files accordingly. Regarding the tests not being mandatory, let's reconsider. Over the last few months, we’ve significantly improved the flaky tests, and making them mandatory would ensure greater confidence that nothing breaks moving forward. 🙏

cc @zone-live @shane-t

image

@Gudahtt Gudahtt dismissed their stale review October 23, 2024 12:36

Requested changes have been made

@metamaskbot
Copy link
Collaborator

Builds ready [e262903]
Page Load Metrics (1935 ± 88 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30724901865398191
domContentLoaded16752403189417484
load16872491193518388
domInteractive206940147
backgroundConnect10149463517
firstReactRender52206924321
getState56323199
initialActions00000
loadScripts12131955141917182
setupStore1298332311
uiStartup189126572170209100
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
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!

@albertolive albertolive added this pull request to the merge queue Oct 25, 2024
Merged via the queue into develop with commit d7d3b6d Oct 25, 2024
76 checks passed
@albertolive albertolive deleted the improve-mmi-e2e-trigger branch October 25, 2024 21:22
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.7.0 Issue or pull request that will be included in release 12.7.0 team-mmi PRs from the MMI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants