Skip to content

Add e2e tests to PR CI workflow #6903

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.

Sign up for GitHub

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

Merged
ryancbahan merged 3 commits into from
Mar 11, 2026
Merged

Add e2e tests to PR CI workflow #6903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
887d4da
Add e2e tests to PR CI workflow
ryancbahan Feb 27, 2026
1584fb4
dont block merge on e2e fail
ryancbahan Mar 5, 2026
07ba445
Fix knip: ignore playwright binary, remove stale ignoreDependencies e…
ryancbahan Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .github/workflows/tests-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,53 @@ jobs:
with:
base-branch-name: '${{ github.base_ref }}'

e2e-tests:
name: 'E2E tests'
if: github.event.pull_request.head.repo.full_name == github.repository
Copy link
Contributor

@dmerand dmerand Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Suggestion: The condition if: github.event.pull_request.head.repo.full_name == github.repository properly prevents PRs from forks from accessing repository secrets (good security practice), but it still allows dependabot PRs to run. Other jobs like 'test-coverage' and 'graphql-schema' skip dependabot PRs since they often don't need full validation. While continue-on-error: true prevents blocking, adding this skip would save CI resources and align with the established pattern.

Suggestion:

Suggested change
if: github.event.pull_request.head.repo.full_name == github.repository
if: github.event.pull_request.head.repo.full_name == github.repository && github.event.pull_request.user.login != 'dependabot[bot]'

runs-on: ubuntu-latest
timeout-minutes: 15
continue-on-error: true
steps:
Comment on lines +226 to +229
Copy link
Contributor

@craigmichaelmartin craigmichaelmartin Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm late to reviewing these, but did we consider using the matrix strategy to verify the e2e across all three operating systems? EG: #6981

This would unlock release paths which are much simpler

- uses: actions/checkout@v3
with:
repository: ${{ github.event.pull_request.head.repo.full_name || github.event.repository.full_name }}
ref: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }}
fetch-depth: 1
- name: Setup deps
uses: ./.github/actions/setup-cli-deps
with:
node-version: ${{ env.DEFAULT_NODE_VERSION }}
- name: Build
run: pnpm nx run-many --all --skip-nx-cache --target=build --output-style=stream
- name: Install Playwright Chromium
run: npx playwright install chromium
working-directory: packages/e2e
- name: Rebuild node-pty
run: pnpm rebuild node-pty
- name: Run E2E tests
working-directory: packages/e2e
env:
SHOPIFY_FLAG_CLIENT_ID: ${{ secrets.E2E_CLIENT_ID }}
Copy link
Contributor

@dmerand dmerand Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI suggested this. I'm logging it as a future improvement once we've vetted this E2E flow:

🔒 Security: The e2e job uses E2E_CLIENT_ID but the global workflow environment already sets SHOPIFY_FLAG_CLIENT_ID from secrets.SHOPIFY_FLAG_CLIENT_ID. The env.ts fixture expects SHOPIFY_FLAG_CLIENT_ID, so the local override works, but this dual-secret pattern is confusing and inconsistent with how acceptance-tests uses secrets. This could lead to confusion about which secret is actually being used and makes the workflow harder to maintain.

Suggestion: Consider whether E2E_CLIENT_ID and SHOPIFY_FLAG_CLIENT_ID should be the same secret or different. If they serve different purposes, document why with a comment. If they should be the same, use consistent naming across jobs to avoid confusion.

Copy link
Contributor Author

@ryancbahan ryancbahan Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local vs hosted secrets for different environments is a common enough pattern. We can look into converging, I'm not sure offhand what GH offers for pulling down env secrets to local but there's probably something there.

E2E_ACCOUNT_EMAIL: ${{ secrets.E2E_ACCOUNT_EMAIL }}
E2E_ACCOUNT_PASSWORD: ${{ secrets.E2E_ACCOUNT_PASSWORD }}
E2E_STORE_FQDN: ${{ secrets.E2E_STORE_FQDN }}
E2E_SECONDARY_CLIENT_ID: ${{ secrets.E2E_SECONDARY_CLIENT_ID }}
run: npx playwright test
- name: Upload Playwright report
uses: actions/upload-artifact@v4
if: ${{ !cancelled() }}
with:
name: playwright-report
path: packages/e2e/playwright-report/
retention-days: 14
- name: Upload test results
uses: actions/upload-artifact@v4
if: ${{ !cancelled() }}
with:
name: playwright-results
path: packages/e2e/test-results/
retention-days: 14

type-diff:
if: github.event.pull_request.head.repo.full_name == github.repository
name: 'Type-diff'
Expand Down
6 changes: 4 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,12 @@
"unlisted": "error",
"unresolved": "error"
},
"ignoreBinaries": [
"playwright"
],
"ignoreDependencies": [
"@shopify/theme-check-node",
"@shopify/theme-check-docs-updater",
"@graphql-typed-document-node/core"
"@shopify/theme-check-docs-updater"
],
"ignoreWorkspaces": [
"packages/eslint-plugin-cli",
Expand Down
Loading