C3: Better e2e quarantine dx

[jculvey]

What this PR solves / how to test:

This PR seeks to improve the dev experience around quarantined e2e tests. Quarantine tests fail sometimes, by design, and this can be confusing if you aren't engaged in c3 development day to day. Ideally, we'd have a system where tests could fail, but we would still get the green check mark that lets us know things are good to merge.

Unfortunately Github Actions doesn't support a allow-failure option for jobs. It's a feature that's been heavily requested (see here and here), though it doesn't appear that Github plans to implement this feature.

This change makes it so that quarantine runs are only performed on merge to master, so PR authors won't see confusing failed builds.

I've also refactored the runCliWithDeploy logic to reduce duplication between quarantined/not-quarantined runs.

Associated docs issue(s)/PR(s):

• [insert associated docs issue(s)/PR(s)]

Author has included the following, where applicable:

• Tests
• Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

• Checked for inclusion of relevant tests
• Checked for inclusion of a relevant changeset
• Checked for creation of associated docs updates
• Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7073f53

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6460178561/npm-package-wrangler-4096

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6460178561/npm-package-wrangler-4096

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6460178561/npm-package-wrangler-4096 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6460178561/npm-package-cloudflare-pages-shared-4096

Note that these links will no longer work once the GitHub Actions artifact expires.

---

wrangler@3.11.0 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	3.20231002.1	3.20231002.1
workerd	1.20231002.0	1.20231002.0
workerd --version	1.20231002.0	2023-10-02

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[dario-piotrowicz]

Tentative approval, I both like and dislike the idea of always passing the tests, at the very least I think it might be worth a try, but hopefully we can keep an eye on the quarantined tests and try to remove them from the quarantine when possible

[dario-piotrowicz]

does gatsby need to be quarantined? I thought you made it better 🤔

[jculvey]

It's better but not better enough quite yet. It's failing somewhat consistently with bun atm and I need to debug why. Don't worry, clearing out the quarantine is next on my list of things to do.

[jculvey]

Took it out of quarantine in another PR: https://github.com/cloudflare/workers-sdk/pull/4120/files

[petebacondarwin]

If we are going to move the e2e quarantined tests to main branch pushes only, then perhaps this PR is not really needed any more?
By the way, I wondered if we could get retries and green-on-failure by adding another step after the two potentially failing steps that just says all is fine?

[jculvey]

If we are going to move the e2e quarantined tests to main branch pushes only, then perhaps this PR is not really needed any more?

This pr contains the changes to run quarantine the tests on main only.

Apart from that, I still think there's value in splitting up the workflow into multiple files and creating a common step for running the e2e since it eliminates redundancy b/w the standard and quarantine runs. It also makes it easier for PR authors to navigate all of the tests run on their PR.

If there's particular things that you don't want included can I maybe remove those and keep the bulk of the refactor?

By the way, I wondered if we could get retries and green-on-failure by adding another step after the two potentially failing steps that just says all is fine?

Yup, that sounds like it could work. If we really want to retain retries that might be the way to go. I still think that there's not a ton of value in running retries on quarantined framework, since there's a high likelihood that they'll fail.

[petebacondarwin]

If we are going to move the e2e quarantined tests to main branch pushes only, then perhaps this PR is not really needed any more?

This pr contains the changes to run quarantine the tests on main only.

Ah! OK. I was confused by this in the PR description:

This change will alter quarantine runs by explicitly catching any errors that occur and reporting it to the user, allowing the test to pass despite it's actual failed state. This allows quarantine runs to pass with all green, but still allows us to inspect the logs to see if quarantined frameworks are actually failing runs.

I don't think we need to make quarantined tests look green if they're not, when they are no longer running on PRs.

[jculvey]

I don't think we need to make quarantined tests look green if they're not, when they are no longer running on PRs.

Fair enough. I just pushed a commit that lets them fail again, with retries.

[codecov]

Codecov Report

Merging #4096 (7073f53) into main (5fc7a88) will increase coverage by 0.29%.
Report is 13 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4096      +/-   ##
==========================================
+ Coverage   75.09%   75.39%   +0.29%     
==========================================
  Files         217      223       +6     
  Lines       12077    12203     +126     
  Branches     3128     3154      +26     
==========================================
+ Hits         9069     9200     +131     
+ Misses       3008     3003       -5     

see 36 files with indirect coverage changes