[HOLD for payment 2023-05-23] Make it so we can run E2E performance tests on any branch

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Problem

When there's a performance regression on staging caught by the automated e2e performance tests, it's not always easy to trace that regression.

Sometimes, we want to just revert suspect PRs just to see if that works, but that gets messy quick if it we then revert the revert.

Solution

Make it so that we can run the E2E performance tests on any branch, not just when a PR is merged.

View all open jobs on GitHub

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.78-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [No QA] Run E2E performance tests imperatively #15608
• [No QA] Remove DELTA_REF input from E2ETests workflow call #15613

If no regressions arise, payment will be issued on 2023-03-13. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

@roryabraham Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

@roryabraham 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[roryabraham]

Worked with ring0 to try and get this working with a larger runner here

[roryabraham]

Some progress made today in that the jobs are running now. It seems there are some build errors, but at least now we can work on fixing them.

[roryabraham]

One way to do this might be to do this to do the test merge locally:

git checkout main
git merge --no-commit $PR_HEAD

Also, we need to set the expected behavior when the PR is unmerged and has merge conflicts. I kind of think in that case we should just fail early.

[AndrewGable]

@roryabraham - Are the tests currently broken?

[roryabraham]

yes

[AndrewGable]

What are we doing to prioritize fixing them? I think they would have caught performance regressions like: #17427

[roryabraham]

I posted about this here last week

[AndrewGable]

I think the tests are going to be fixed by this PR: #17569

The only issue now is that we need to wait for a deploy cycle to fix our comparison release to the PR version it compares against.

[jczekalski]

@roryabraham The simplest solution I found to fix the issue is to use the last commit in the PR instead of the merge commit if the PR hasn't been merged yet:

getPullRequestDetails.js

-        core.setOutput('MERGE_COMMIT_SHA', PR.merge_commit_sha);
+        core.setOutput('MERGE_COMMIT_SHA', PR.state === 'open' ? PR.head.sha : PR.merge_commit_sha);

This is enough to get the "Build APK from delta ref" job to complete successfully. I'm not sure about the AWS tests since those will always fail because I'm running the workflow from my fork (Credentials could not be loaded).

I'm also testing out the "merge PR locally, then get and checkout merge commit sha" approach you mentioned, but I don't have a working solution yet.

[jczekalski]

Update: I think the solution I posted above only worked because I was running the workflow from my fork against my own PR in the upstream repo. Running it against other PRs results in the same invalid commit error message.

I tried this approach and also get the same message when trying checkout/merge the PR head commit:

git checkout main
git merge --no-commit $PR_HEAD_SHA

Though perhaps this is because I'm running the workflow from a forked repo?

In the end I managed to get it working (for branches from forked repositories) by adding the forked repo as a remote and fetching the changes before merging:

git checkout main
git remote add pr_remote $PR_HEAD_REPO_URL
git fetch pr_remote
git merge --no-commit $PR_HEAD_COMMIT_SHA
echo "MERGE_COMMIT_SHA=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"

This seemed to do the trick, but after a while I started getting Committer identity unknown error messages at the git merge step and now I'm confused again. I'll look into this tomorrow. I find this task pretty difficult to work on in general since I can't run the workflow from the main repo, but maybe it's just a matter of getting used to this setup (which is also quite new to me).

[jczekalski]

Update: I opened a PR with my solution: #17972

When running the e2e performance tests workflow for an unmerged PR, the "delta ref" is checked out and the apks are built and downloaded successfully.

Examples of the workflow working correctly:
Unmerged internal PR: https://github.com/Expensify/App/actions/runs/4886666904
Unmerged external PR: https://github.com/Expensify/App/actions/runs/4886668220
Merged PR: https://github.com/Expensify/App/actions/runs/4886670026

All 3 workflows now fail at the Schedule AWS Device Farm test run step with this error:

Error: Unable to publish app file zip/app-e2eRelease-baseline.apk, Unable to download remote file zip/app-e2eRelease-baseline.apk to app-e2eRelease-baseline.apk, Error: connect ECONNREFUSED 127.0.0.1:80

I’m not sure if I should continue working on this issue since as an external contributor I can only run the workflow from my fork, which means it will always fail at Configure AWS Credentials.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.14-14 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix/15605: Run E2E performance tests on any branch #17972

If no regressions arise, payment will be issued on 2023-05-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

@jczekalski, @roryabraham Huh... This is 4 days overdue. Who can take care of this?

[roryabraham]

This was completed by SWM w/o an additional bounty, so no additional payment is due here.