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

Enable E2E in merge queue #11747

Open
9 tasks
cortisiko opened this issue Oct 10, 2024 · 5 comments
Open
9 tasks

Enable E2E in merge queue #11747

cortisiko opened this issue Oct 10, 2024 · 5 comments
Assignees
Labels
team-dev-ops DevOps team team-mobile-platform Mobile Platform team

Comments

@cortisiko
Copy link
Member

cortisiko commented Oct 10, 2024

What is this about?

To improve the stability and reliability of our main branch, we are implementing a strategy to reduce the number of PRs that introduce failed E2E tests. Our goal is to enable E2E testing by default for any PR that enters the merge queue, ensuring that only thoroughly validated code makes it into the main branch. This will help catch potential issues early, prevent broken code from being merged, and increase overall confidence in our CI pipeline.

The Approach:

  • Keep the current logic where we do not require the e2e check on feature branches.

  • Enforce E2E Checks in the Merge Queue: We should set a branch protection rule that requires E2E tests to run on all PRs that are added to the merge queue. This ensures that every PR is subject to automated testing before it reaches main, reducing the likelihood of undetected issues.

  • Skip E2E for Documentation PRs: To avoid wasting resources, the E2E tests will be automatically skipped for PRs whose titles contain "docs." This helps streamline the process for non-code changes, ensuring that documentation updates can be merged quickly without unnecessary testing overhead. The below job was created to verify whether a PR follows the conventional naming format by including "docs" in its title.

 is-documentation-pull-request:
    name: Check if pull request is for documentation
    runs-on: ubuntu-latest
    outputs:
      IS_DOCS_PR: ${{ steps.is-docs.outputs.IS_DOCS_PR }}
    steps:
      - uses: actions/checkout@v3
      - name: Check if docs PR
        id: is-docs
        run: |
          if [[ "${{ startsWith(github.event.pull_request.title, 'docs:') }}" == "true" ]]; then
            echo "Is a documentation PR, will skip running e2e"
          else
            echo "Is not a documentation PR will proceed to running tests"
            fi

Please feel free to use this if you see fit.

Here is PR that introduced the e2e check in GitHub Actions: #8495

Scenario

No response

Design

No response

Technical Details

No response

Acceptance Criteria

No response

Stakeholder review needed before the work gets merged

  • Engineering (needed in most cases)
  • Design
  • Product
  • QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
  • Security
  • Legal
  • Marketing
  • Management (please specify)
  • Other (please specify)

References

No response

@cortisiko cortisiko self-assigned this Oct 10, 2024
@metamaskbot metamaskbot added the INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. label Oct 22, 2024
@cortisiko cortisiko added team-dev-ops DevOps team and removed technical research INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. labels Oct 22, 2024
@jake-perkins
Copy link
Contributor

The story ask is to run the e2e in the merge queue and I have been able to isolate some ci run flows for just merge queue process and run the e2e invocation points.

However the way MQ works it actually makes an alternate read-only branch distinct and separate from the originating PR that was added to MQ with different commit shas/branch names which is unfortunate. the e2e bitrise process involves several mutating requests to both GH API/BitRise API

I don't think it is feasible given some of the constraints around merge_group event being separate/distinct from pull_request event
for example here an example webhook payload for MQ ( merge_group )

{"base_ref":"refs/heads/tester","base_sha":"a9e5e81f11be20533feecccdeaa6fd3c94cee6be","head_commit":{"author":{"email":"128608287+jake-perkins@users.noreply.github.com","name":"jake-perkins"},"committer":{"email":"noreply@github.com","name":"GitHub"},"id":"fb3feb4e819347d00eebc2ea058bf54889d6027d","message":"Merge pull request #5 from MetaMask/mq-checker\n\nchecker","timestamp":"2024-11-08T04:00:32Z","tree_id":"2f88d615e79f27ef6ca57c411915bac40e9fb925"},
"head_ref":"refs/heads/gh-readonly-queue/tester/pr-5-a9e5e81f11be20533feecccdeaa6fd3c94cee6be","head_sha":"fb3feb4e819347d00eebc2ea058bf54889d6027d"}

Basically it spins up a secondary special ( gh-readonly-queue/ ) branch and all the shas are different which will break some of the linkage between Bitrise /GH. and all of the mutating API calls will fail to GH

Option 1 :
One recommendation/alternate suggested was to simply make the status check a required check on the branch protection rule for main
- One potential unknown here is if the check being assigned to a random check bucket will impact
the gate, we can quickly test on this one though
( CLA Signature Bot / Bitrise E2E Status (pull_request_target) ) is how it currently shows

Option 2:
Auto Draft PRs on Open + Run E2E checks on Ready to Review PRs only

@jake-perkins
Copy link
Contributor

#12243

github-merge-queue bot pushed a commit that referenced this issue Dec 3, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

This PR does a few things
1. Automatically set new PRs targeting main to "draft" state.
2. Enforce the E2E Bitrise Status Check on lifecycle on incoming PRs
which will essentially enforce that the PR has 1 of the two labels 'Run
Smoke E2E' or 'No E2E Smoke Needed'


<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
4. What is the improvement/solution?
-->

With this change we're looking to improve the posture on our PR
lifecycle with auto drafts and enforce a higher standard for incoming
changes towards or E2E testing posture.

## **Related issues**

[Fixes:]

#11747

## **Manual testing steps**

UC1 : Nether E2E Label is set
->
https://github.com/MetaMask/metamask-mobile/actions/runs/11826040868/job/32951145968?pr=12243
    Expected : Not Mergeable

UC2 : docs PR

Should run: false, Reason: The pull request is documentation related.
Skipping Bitrise status check. due to the following reason: The pull
request is documentation related.
Created 'Bitrise E2E Status' check with skipped status for commit
dea08a9

https://github.com/MetaMask/metamask-mobile/actions/runs/11826187817/job/32951608300?pr=12243

Expected: Mergeable

UC3 : No E2E Smoke Needed Label is set

Anti label: true
Should run: false, Reason: The pull request has the anti-label.
Skipping Bitrise status check. due to the following reason: The pull
request has the anti-label.
Created 'Bitrise E2E Status' check with skipped status for commit
dea08a9[22]

https://github.com/MetaMask/metamask-mobile/actions/runs/11826234735/job/32951749305?pr=12243

Expected Mergeable


UC4: Smoke E2E Label is set

Should run: true, Reason: The smoke test label is present.
Starting Bitrise build for commit
dea08a9


## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

N/A CICD Changes only

### **After**

<!-- [screenshots/recordings] -->

N/A CICD Changes only

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.

---------

Co-authored-by: Cal Leung <cal.leung@consensys.net>
@jake-perkins
Copy link
Contributor

Latest approach has showed very promising results using the checks CRUD API - PR here #12648

@gauthierpetetin
Copy link
Contributor

Hi @jake-perkins , what is the latest status of this issue? Is it on purpose that its status is set to "closed" on our Github Project board?

@gauthierpetetin
Copy link
Contributor

Almost done, will be wrapped up tomorrow

@jake-perkins jake-perkins mentioned this issue Mar 6, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-dev-ops DevOps team team-mobile-platform Mobile Platform team
Projects
None yet
Development

No branches or pull requests

4 participants