[GitHub Workflow] Fix e2e test + cache node_modules#6500
Conversation
|
That's weird, it is still failed at the same point. It looks like we didn't change anything else. |
louislam
force-pushed
the
revert-6491-fix/tag-ui-bug
branch
from
0960d1d to
09017fb
Compare
louislam
changed the title
There was a problem hiding this comment.
Pull request overview
This PR addresses e2e test failures caused by changes to example.com's NS records and implements node_modules caching across GitHub workflows to improve CI performance.
Key Changes:
- Updates DNS NS test data from example.com to kuma.pet with corresponding nameserver values
- Adds node_modules caching to all workflow jobs using actions/cache@v4
- Restructures job dependencies to run e2e-test before other test jobs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| test/e2e/specs/monitor-form.spec.js | Updates DNS NS test to use kuma.pet domain with Cloudflare nameservers instead of example.com with IANA servers |
| .github/workflows/auto-test.yml | Adds node_modules caching, changes job dependencies, updates e2e-test runner to ARM64, pins Playwright version, and switches from npm ci to npm install |
| .github/workflows/close-incorrect-issue.yml | Removes npm cache configuration from setup-node action |
| - name: Cache/Restore node_modules | ||
| uses: actions/cache@v4 | ||
| id: node-modules-cache | ||
| with: | ||
| path: node_modules | ||
| key: node-modules-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} | ||
|
|
There was a problem hiding this comment.
The caching strategy is incomplete. After restoring the cache, the workflow should check if the cache was hit and only run npm ci when the cache misses. Without this check, npm install will run every time, which defeats the purpose of caching. Add a condition like if: steps.node-modules-cache.outputs.cache-hit != 'true' to the npm install step.
| with: | ||
| node-version: ${{ matrix.node }} | ||
| - run: npm ci --production | ||
| - run: npm install --production |
There was a problem hiding this comment.
The comment is now inaccurate. The job has been changed to use npm install --production instead of npm ci --production, but the comment still references the old command. Update the comment to reflect the actual command being used.
| - name: Cache/Restore node_modules | ||
| uses: actions/cache@v4 | ||
| id: node-modules-cache | ||
| with: | ||
| path: node_modules | ||
| key: node-modules-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} | ||
|
|
There was a problem hiding this comment.
The caching strategy is incomplete. After restoring the cache, the workflow should check if the cache was hit and only run npm install when the cache misses. Without this check, npm install will run every time, which defeats the purpose of caching. Add a condition like if: steps.node-modules-cache.outputs.cache-hit != 'true' to the npm install step.
| - name: Cache/Restore node_modules | ||
| uses: actions/cache@v4 | ||
| id: node-modules-cache | ||
| with: | ||
| path: node_modules | ||
| key: node-modules-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} |
There was a problem hiding this comment.
The caching strategy is incomplete. After restoring the cache, the workflow should check if the cache was hit and only run npm install when the cache misses. Without this check, npm install will run every time, which defeats the purpose of caching. Add a condition like if: steps.node-modules-cache.outputs.cache-hit != 'true' to the npm install step.
| # As a lot of dev dependencies are not supported on ARMv7, we have to test it separately and just test if `npm ci --production` works | ||
| armv7-simple-test: | ||
| needs: [ ] | ||
| needs: [ e2e-test ] |
There was a problem hiding this comment.
Changing the dependency chain creates a problematic workflow sequence. The armv7-simple-test job now depends on e2e-test, which depends on check-linters. This means the armv7 test must wait for both linters and e2e tests to complete before running, which significantly increases CI time and prevents parallel execution. Consider whether this dependency is necessary or if jobs should run in parallel.
| needs: [ e2e-test ] |
| needs: [ check-linters ] | ||
| runs-on: ARM64 | ||
| env: | ||
| PLAYWRIGHT_VERSION: ~1.39.0 |
There was a problem hiding this comment.
The tilde version specifier ~1.39.0 in Playwright version will only match patch updates (1.39.x) but this version is quite old. Playwright 1.39 was released in late 2023. Consider using a more recent version or a caret specifier (^1.39.0) to allow minor updates, or specify the exact version if stability is critical.
| PLAYWRIGHT_VERSION: ~1.39.0 | |
| PLAYWRIGHT_VERSION: ^1.49.0 |
| - name: Install Playwright ${{ env.PLAYWRIGHT_VERSION }} | ||
| run: npx playwright@${{ env.PLAYWRIGHT_VERSION }} install |
There was a problem hiding this comment.
The Playwright installation command is incorrect. Using npx playwright@${{ env.PLAYWRIGHT_VERSION }} install installs the Playwright browsers for the specified version, but it doesn't install the Playwright package itself at that version. This command should be run after npm install (which already installs Playwright from package.json), or you should use npx playwright install without the version specifier since the version is already controlled by package.json.
| needs: [ ] | ||
| runs-on: ubuntu-24.04-arm | ||
| needs: [ check-linters ] | ||
| runs-on: ARM64 |
There was a problem hiding this comment.
The runner label ARM64 may not be a valid GitHub-hosted runner. GitHub's standard hosted runners are labels like ubuntu-latest, ubuntu-24.04, macos-latest, etc. The previous value ubuntu-24.04-arm was more specific. Verify that ARM64 is a valid self-hosted runner label in your organization, otherwise this job will fail to find a runner.
| - name: Cache/Restore node_modules | ||
| uses: actions/cache@v4 | ||
| id: node-modules-cache | ||
| with: | ||
| path: node_modules | ||
| key: node-modules-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} | ||
|
|
There was a problem hiding this comment.
The caching strategy is incomplete. After restoring the cache, the workflow should check if the cache was hit and only run npm install when the cache misses. Without this check, npm install will run every time, which defeats the purpose of caching. Add a condition like if: steps.node-modules-cache.outputs.cache-hit != 'true' to the npm install step.
| jobs: | ||
| auto-test: | ||
| needs: [ check-linters ] | ||
| needs: [ e2e-test ] |
There was a problem hiding this comment.
Changing the dependency chain creates a circular dependency problem. The auto-test job now depends on e2e-test, but e2e-test depends on check-linters. This means all tests must wait for e2e-test to complete before running, which significantly increases CI time and prevents parallel execution. Consider whether this dependency is necessary or if jobs should run in parallel.
| needs: [ e2e-test ] |
This pr is just used to investigate, not meant to be merged.Not sure if some kind of race condition after #6491.
https://github.com/louislam/uptime-kuma/actions/runs/20300273022/job/58303870710
Edit:
This is because example.com had changed their NS record.