-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Run more tests, more of the time #9737
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
Changes from all commits
6845930
08189c0
96b28c1
70a8c94
7d07dd2
082238e
193503f
20a70d0
c0e9e7e
400fecf
d65473b
ae741b7
0aa66ba
a693002
5d42b89
8d729e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,19 +10,12 @@ The following selections do not need to be completed if this PR only contains ch | |
| --> | ||
|
|
||
| - Tests | ||
| - [ ] TODO (before merge) | ||
| - [ ] Tests included | ||
| - [ ] Tests not necessary because: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Samuel mentioned in the description that 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've reverted this change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
External contributors can't add the label though 😕
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) 🤔 |
||
| - 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): <!--e.g. <https://github.com/cloudflare/cloudflare-docs/pull/>...--> | ||
| - [ ] Documentation not necessary because: | ||
| - Wrangler V3 Backport | ||
| - [ ] TODO (before merge) | ||
| - [ ] Wrangler PR: <!--e.g. <https://github.com/cloudflare/workers-sdk/pull/>...--> | ||
| - [ ] Not necessary because: <!--e.g. not a patch change, not a Wrangler change...--> | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,127 +1,131 @@ | ||
| name: E2E tests | ||
| name: E2E | ||
dario-piotrowicz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| on: | ||
| merge_group: | ||
| pull_request: | ||
| types: [synchronize, opened, reopened, labeled, unlabeled] | ||
| repository_dispatch: | ||
|
|
||
| jobs: | ||
| e2e-test-vp: | ||
| name: "E2E tests" | ||
| e2e-wrangler-test: | ||
| 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 | ||
| 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: Bump package versions | ||
| run: node .github/changeset-version.js | ||
| env: | ||
| GITHUB_TOKEN: ${{ github.token }} | ||
|
|
||
| - name: Run Vite E2E tests | ||
| if: github.event.pull_request.head.repo.owner.login == 'cloudflare' | ||
| run: pnpm test:e2e -F @cloudflare/vite-plugin --log-order=stream | ||
| timeout-minutes: 15 | ||
| 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 }} | ||
|
|
||
| - name: Run Wrangler E2E tests | ||
| 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 | ||
Check warningCode scanning / CodeQL Workflow does not contain permissions Medium
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉