Run more tests, more of the time #9737

penalosa · 2025-06-25T01:19:48Z

One of the most painful category of release issues is Something is wrong in VP that could have been caught earlier. e.g.

A PR was merged that didn’t run E2E tests but that broke them
A PR was merged which combined badly with something else already on main

This PR aims to address that pain by running all tests (including E2E tests) in the Merge Queue, which we already enforce as a merge strategy. That will mean that forks can run E2E tests (since at the point a fork PR is in the Merge Queue it’s been approved, so can be trusted to run E2E tests with tokens). It will also mean that nothing lands in main without fully passing E2E tests, having been rebased on top of the tip of main. This also means we no longer have a split between PRs and VP in terms of what OS tests are run on.

There are a couple of things this PR has done to make this work well:

Some hacking around of GH Actions to get things properly marked as required even if they’re skipped (basically, how can we make E2E tests a required check without completely breaking forks?). To do this, I've changed the stage at which runs that will be skipped are skipped—in exactly the same way as we do for markdown-only PRs, where CI tests don’t properly “run” but they do “run” from the perspective of GH Actions.
Apply pending changeset version bumps to packages before running tests. Every so often we get test failures on VP that didn’t show up on PRs because VP has bumped the version numbers in package.json files according to the pending changesets publish. This PR also applies this pending version update to the package.json files in packages before running tests on all workflows. This also has the benefit of allowing test runs on VP to hit cached values.

This PR also simplifies the pull request template by:

Removing the TODO checkboxes
Removing the E2E checkboxes (since now they always run)
Changing the tests checkbox to only have the option to mark "Tests included". This can be bypassed with the no-tests label, but hopefully removing the visible option for not including tests will encourage people to include tests

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because:
Wrangler / Vite E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: internal
Wrangler V3 Backport
- TODO (before merge)
- Wrangler PR:
- Not necessary because: not a Wrangler change

changeset-bot · 2025-06-25T01:19:52Z

⚠️ No Changeset found

Latest commit: 8d729e1

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

pkg-pr-new · 2025-06-25T01:22:07Z

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@9737

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@9737

miniflare

npm i https://pkg.pr.new/miniflare@9737

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@9737

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@9737

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@9737

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@9737

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@9737

wrangler

npm i https://pkg.pr.new/wrangler@9737

commit: 8d729e1

.github/workflows/e2e.yml

emily-shen · 2025-06-25T14:26:44Z

fixtures/interactive-dev-tests/package.json

 	},
 	"optionalDependencies": {
-		"@cdktf/node-pty-prebuilt-multiarch": "0.10.1-pre.11"
+		"@cdktf/node-pty-prebuilt-multiarch": "0.10.2"


why this change?

Just a version bump while trying to get this fixture working in later versions of Node. Turns out it doesn't support them though (see cdktf/node-pty-prebuilt-multiarch#119), but there's no harm in using a slightly newer version

emily-shen · 2025-06-25T14:28:05Z

.github/workflows/test-and-check.yml

-      - name: Check for errors
-        run: pnpm run check --summarize
+      - name: Bump package versions
+        run: node .github/changeset-version.js


is there anything to worry about here re: order of merges? is this on every workflow just in case, or could they conflict somehow? i guess i don't really understand why different versions has caused failures in VP/what sort of failures.

This won't affect the actual code that is merged from a PR, it just ensures that the tests run against the PR as if it was on VP (with the version bumps applied). This is both to prevent problems (e.g. C3 inadvertently works because the published version of Wrangler is the same as the version on disk, but fails for some reason if they're different, which has happened in the past), but also to fix turbo caches so that VP can be cached. Currently VP is completely uncached because all the package.json files change, which busts the cache

emily-shen · 2025-06-25T14:31:38Z

.github/pull_request_template.md

dario-piotrowicz · 2025-06-25T16:13:45Z

.github/pull_request_template.md

 - Tests
-  - [ ] TODO (before merge)
  - [ ] Tests included
-  - [ ] Tests not necessary because:


did you mean to remove this checkbox? this change doesn't seem intentional to me? 🤔

(test being the only checkbox without the not-necessary checkbox)

Samuel mentioned in the description that Changing the tests checkbox to only have the option to mark "Tests included". This can be bypassed with the no-tests label.

But I wonder if it's better to keep the no test checkbox as we lost the context on why if we rely on the label.

I see... yeah but the way I would expect it, similarly to how V3 backporting works, is that we do have the check and also the label 🤔

The description had my motivation here: "hopefully removing the visible option for not including tests will encourage people to include tests", but happy to revert this. My thinking was that basically all PRs should have tests, and it should be fairly hidden option to not add tests. It'll also be much more obvious for us when reviewing that a PR has the no-tests label, which should make it easier to spot if something has been inadvertently marked as not requiring tests.

I've reverted this change

It'll also be much more obvious for us when reviewing that a PR has the no-tests label, which should make it easier to spot if something has been inadvertently marked as not requiring tests.

External contributors can't add the label though 😕

My thinking was that basically all PRs should have tests

I agree if we're talking about actual code changes, but often there other types of changes that don't require tests, such as code comments, changeset changes, templates changes, infra changes, md changes (which we do have a comment about in the pr template anyways) 🤔

.github/workflows/c3-e2e.yml

dario-piotrowicz · 2025-06-25T16:19:37Z

.github/workflows/c3-e2e.yml

 on:
+  merge_group:
  pull_request:
    types: [synchronize, opened, reopened, labeled, unlabeled]


if we're not checking for labels anymore shouldn't we remove labeled/unlabeled from here? (we don't need to re-run the workflow when labels change, right?)

(this also applies to the other workflows)

dario-piotrowicz · 2025-06-25T16:26:58Z

.github/workflows/c3-e2e.yml

      group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}-${{ matrix.pm.name }}-${{ matrix.pm.version }}
      cancel-in-progress: true
-    name: ${{ format('Run tests for {0}@{1} on {2}', matrix.pm.name, matrix.pm.version, matrix.os) }}
+    name: ${{ format('C3 ({0})', matrix.label) }}


I would really avoid using the word label used for something different from what GitHub refers as labels, sounds to me like something bound to cause confusion at some point

(this also applies to the other workflows)

dario-piotrowicz

I've left a few comments, but nothing blocking, looks good to me 🙂

.github/workflows/c3-e2e.yml

.github/workflows/e2e.yml

+    name: ${{ format('Wrangler ({0})', matrix.description) }}
    concurrency:
-      group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}-${{ matrix.node }}
+      group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}-${{ matrix.node }}-wrangler
      cancel-in-progress: true
    timeout-minutes: 60
-    if: github.event_name == 'pull_request' && contains(github.event.*.labels.*.name, 'e2e' ) && github.event.pull_request.head.repo.owner.login == 'cloudflare' && (github.head_ref == 'changeset-release/main' || contains(github.event.*.labels.*.name, 'every-os' ))
    strategy:
      fail-fast: false
      matrix:
-        os: [macos-13, windows-2022]
-        node: ["20.19.1"]
+        include:
+          - os: macos-13
+            description: v20, macOS
+            node: 20.19.1
+          - os: windows-2022
+            description: v20, Windows
+            node: 20.19.1
+          - os: ubuntu-22.04-arm
+            description: v20, Linux
+            node: 20.19.1
+
    runs-on: ${{ matrix.os }}
    steps:
      - name: Checkout Repo
+        if: github.event.pull_request.head.repo.owner.login == 'cloudflare'
        uses: actions/checkout@v4
        with:
          fetch-depth: 0

      - name: Install Dependencies
+        if: github.event.pull_request.head.repo.owner.login == 'cloudflare'
        uses: ./.github/actions/install-dependencies
        with:
          node-version: ${{ matrix.node }}
          turbo-api: ${{ secrets.TURBO_API }}
          turbo-team: ${{ secrets.TURBO_TEAM }}
          turbo-token: ${{ secrets.TURBO_TOKEN }}
          turbo-signature: ${{ secrets.TURBO_REMOTE_CACHE_SIGNATURE_KEY }}

-      - name: Run Vite E2E tests
-        run: pnpm test:e2e -F @cloudflare/vite-plugin --log-order=stream
-        timeout-minutes: 15
+      - name: Bump package versions
+        run: node .github/changeset-version.js
        env:
-          NODE_DEBUG: "vite-plugin:test"
-          # The AI tests need to connect to Cloudflare
-          CLOUDFLARE_API_TOKEN: ${{ secrets.TEST_CLOUDFLARE_API_TOKEN }}
-          CLOUDFLARE_ACCOUNT_ID: ${{ secrets.TEST_CLOUDFLARE_ACCOUNT_ID }}
-          NODE_OPTIONS: "--max_old_space_size=8192"
-          CI_OS: ${{ matrix.os }}
+          GITHUB_TOKEN: ${{ github.token }}

      - name: Run Wrangler E2E tests
+        if: github.event.pull_request.head.repo.owner.login == 'cloudflare'
        run: pnpm run test:e2e:wrangler
        env:
          CLOUDFLARE_API_TOKEN: ${{ secrets.TEST_CLOUDFLARE_API_TOKEN }}
          CLOUDFLARE_ACCOUNT_ID: ${{ secrets.TEST_CLOUDFLARE_ACCOUNT_ID }}
          HYPERDRIVE_DATABASE_URL: ${{ secrets.TEST_HYPERDRIVE_DATABASE_URL}}
          WRANGLER: node --no-warnings ${{ github.workspace}}/packages/wrangler/bin/wrangler.js
          WRANGLER_IMPORT: ${{ github.workspace}}/packages/wrangler/wrangler-dist/cli.js
          MINIFLARE_IMPORT: ${{ github.workspace}}/packages/miniflare/dist/src/index.js
          NODE_OPTIONS: "--max_old_space_size=8192"
          WRANGLER_LOG_PATH: ${{ runner.temp }}/wrangler-debug-logs/
          TEST_REPORT_PATH: ${{ runner.temp }}/test-report/index.html
          CI_OS: ${{ matrix.os }}

      - name: Upload turbo logs
        if: always()
        uses: actions/upload-artifact@v4
        with:
          name: turbo-runs-${{ matrix.os }}-${{ matrix.node }}
          path: .turbo/runs

-  e2e-test:
-    name: "E2E tests"
+  e2e-vite-test:
+    name: ${{ format('Vite ({0})', matrix.description) }}
    concurrency:
-      group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}-${{ matrix.node }}
+      group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}-${{ matrix.node }}-vite
      cancel-in-progress: true
    timeout-minutes: 60
-    if: github.event_name == 'pull_request' && contains(github.event.*.labels.*.name, 'e2e' ) && github.event.pull_request.head.repo.owner.login == 'cloudflare'
    strategy:
-      fail-fast: false
      matrix:
-        os: [ubuntu-22.04-arm]
-        node: ["20.19.1"]
+        include:
+          - os: macos-13


To fix the issue, we will add a permissions block at the root of the workflow file. This block will define the minimal permissions required for the workflow to function correctly. Based on the workflow's operations, the following permissions are necessary:

contents: read for accessing repository contents.

contents: write for the "Bump package versions" step, which modifies repository files.

The permissions block will be added at the root level to apply to all jobs in the workflow. If any job requires additional permissions, they can be overridden within the specific job.

run more tests, more of the time

6845930

penalosa requested a review from a team as a code owner June 25, 2025 01:19

workers-devprod added this to workers-sdk Jun 25, 2025

github-project-automation bot moved this to Untriaged in workers-sdk Jun 25, 2025

Checks can be single-os

08189c0

penalosa added 3 commits June 25, 2025 02:23

syntax

96b28c1

syntax

70a8c94

add more labels

7d07dd2

github-advanced-security bot found potential problems Jun 25, 2025

View reviewed changes

.github/workflows/e2e.yml Fixed Show fixed Hide fixed

penalosa added 4 commits June 25, 2025 02:42

fix group

082238e

fix group

193503f

skip node 24

20a70d0

skip test

c0e9e7e

penalosa added the skip-v3-pr label Jun 25, 2025

Merge branch 'main' into penalosa/more-tests

400fecf

penalosa added no-tests and removed no-tests labels Jun 25, 2025

penalosa added 3 commits June 25, 2025 13:27

Simplify PR validation

d65473b

fix tool tests

ae741b7

re-skip 24

0aa66ba

penalosa added the skip-pr-description-validation Skip validation of the required PR description format label Jun 25, 2025

emily-shen reviewed Jun 25, 2025

View reviewed changes

penalosa requested a review from emily-shen June 25, 2025 14:47

emily-shen approved these changes Jun 25, 2025

View reviewed changes

github-project-automation bot moved this from Untriaged to Approved in workers-sdk Jun 25, 2025

dario-piotrowicz reviewed Jun 25, 2025

View reviewed changes

edmundhung approved these changes Jun 25, 2025

View reviewed changes

dario-piotrowicz approved these changes Jun 25, 2025

View reviewed changes

.github/workflows/c3-e2e.yml Show resolved Hide resolved

.github/workflows/e2e.yml Show resolved Hide resolved

penalosa added 2 commits June 26, 2025 10:52

address review comments

a693002

Merge remote-tracking branch 'origin/main' into penalosa/more-tests

5d42b89

github-advanced-security bot found potential problems Jun 26, 2025

View reviewed changes

match existing test names

8d729e1

penalosa enabled auto-merge June 26, 2025 10:36

penalosa added this pull request to the merge queue Jun 26, 2025

Merged via the queue into main with commit 6fb8f50 Jun 26, 2025
29 checks passed

penalosa deleted the penalosa/more-tests branch June 26, 2025 10:43

github-project-automation bot moved this from Approved to Done in workers-sdk Jun 26, 2025

petebacondarwin mentioned this pull request Aug 13, 2025

backport: improve e2e testing approach to reduce flakiness #10309

Merged

8 tasks

@@ -2,2 +2,5 @@
+            permissions:
+              contents: write
             on:

Run more tests, more of the time #9737

Run more tests, more of the time #9737

Uh oh!

Conversation

penalosa commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

pkg-pr-new bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Check warning

Copilot Autofix

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

penalosa commented Jun 25, 2025 •

edited

Loading

changeset-bot bot commented Jun 25, 2025 •

edited

Loading

pkg-pr-new bot commented Jun 25, 2025 •

edited

Loading