Add Playwright UI tests, test harness and CI workflow - #27
Conversation
|
🌸 Diff Haiku UI tests arrive, Generated by spit-the-diff |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds Playwright UI tests to the Shandoku project, along with a test harness and a CI workflow. The goal is to improve the app's reliability by automatically testing core gameplay and Rift flows, and to prevent regressions by running these tests on every push and pull request. The changes include adding Playwright configuration, test files, a test API, and a CI workflow file, as well as updating the package.json and .gitignore files. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Playwright test suite, a test harness, and a CI workflow, which is a significant improvement for ensuring UI stability and catching regressions. The tests are well-structured, covering smoke, mobile, and specific gameplay scenarios. The test harness exposed on the window object is a practical approach for setting up deterministic states. My review includes a few suggestions to enhance code robustness and maintainability, such as using URLSearchParams for more reliable query parameter handling and centralizing common test setup logic to reduce duplication.
| const RIFT_STATUS_DELAY_REDUCED_MOTION_MS = 500; | ||
| const RIFT_EVALUATE_DEBOUNCE_MS = 140; | ||
| const RIFT_NON_CONFLICT_CHECK_INTERVAL = 5; | ||
| const TEST_MODE = window.location.search.includes('e2e=1'); |
There was a problem hiding this comment.
Using window.location.search.includes('e2e=1') can be brittle as it might lead to false positives (e.g., with a URL like ?some_other_param_e2e=1). A more robust and standard approach is to use the URLSearchParams API, which is specifically designed for parsing URL query strings.
| const TEST_MODE = window.location.search.includes('e2e=1'); | |
| const TEST_MODE = new URLSearchParams(window.location.search).has('e2e=1'); |
| function normalizeTestBoard(board, fallbackValue=0){ | ||
| if(!Array.isArray(board)||board.length!==GRID_SIZE){ | ||
| return Array.from({length:GRID_SIZE},()=>Array.from({length:GRID_SIZE},()=>fallbackValue)); | ||
| } | ||
| return board.map(row=>{ | ||
| if(!Array.isArray(row)||row.length!==GRID_SIZE) return Array.from({length:GRID_SIZE},()=>fallbackValue); | ||
| return row.map(value=>{ | ||
| const n=Number(value); | ||
| if(Number.isInteger(n)&&n>=0&&n<=9) return n; | ||
| return fallbackValue; | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| function applyTestBoardState(payload={}){ | ||
| const nextGrid=normalizeTestBoard(payload.grid,0); | ||
| const nextStartingGrid=normalizeTestBoard(payload.startingGrid,0); | ||
| grid=cloneGrid(nextGrid); | ||
| startingGrid=cloneGrid(nextStartingGrid); | ||
| notes=Array.from({length:GRID_SIZE},()=>Array.from({length:GRID_SIZE},()=>new Set())); | ||
| selected=(payload.selected&&Number.isInteger(payload.selected.r)&&Number.isInteger(payload.selected.c)) | ||
| ?{r:Math.max(0,Math.min(8,payload.selected.r)),c:Math.max(0,Math.min(8,payload.selected.c))} | ||
| :null; | ||
| elapsed=normalizeElapsed(payload.elapsed); | ||
| notesMode=!!payload.notesMode; | ||
| autoCleanup=payload.autoCleanup!==false; | ||
| history=[]; future=[]; | ||
| boardShellEl.classList.remove('victory-glow'); | ||
| clearRiftVisualState(); | ||
| riftState={active:false,sequenceRunning:false,nodes:[],hasTriggered:false,cooldownUntil:0,copyKey:'pattern'}; | ||
| movesSinceSolvabilityCheck=0; | ||
| captureLastSolvableSnapshot(); | ||
| render(); | ||
| saveGame(); | ||
| } | ||
|
|
||
| function exposeTestApi(){ | ||
| window.__shandokuTest={ | ||
| resetStorage(){ | ||
| localStorage.removeItem(STORAGE_KEY); | ||
| }, | ||
| setBoardState(payload){ | ||
| applyTestBoardState(payload); | ||
| }, | ||
| getState(){ | ||
| return { | ||
| grid:cloneGrid(grid), | ||
| startingGrid:cloneGrid(startingGrid), | ||
| selected:selected?{...selected}:null, | ||
| notesMode, | ||
| autoCleanup, | ||
| errorCount:countErrors(), | ||
| status:statusEl.textContent | ||
| }; | ||
| }, | ||
| selectCell(r,c){ | ||
| selected={r:Math.max(0,Math.min(8,r)),c:Math.max(0,Math.min(8,c))}; | ||
| render(); | ||
| saveGame(); | ||
| }, | ||
| placeNumber(n){ | ||
| placeNumber(n); | ||
| }, | ||
| newGame(){ | ||
| newGame(); | ||
| }, | ||
| forceRift(payload={}){ | ||
| const r=Number.isInteger(payload.r)?Math.max(0,Math.min(8,payload.r)):4; | ||
| const c=Number.isInteger(payload.c)?Math.max(0,Math.min(8,payload.c)):4; | ||
| clearRiftVisualState(); | ||
| riftState.active=true; | ||
| riftState.sequenceRunning=false; | ||
| riftState.nodes=[{r,c}]; | ||
| riftState.hasTriggered=true; | ||
| riftState.cooldownUntil=Date.now()+RIFT_COOLDOWN_MS; | ||
| boardShellEl.classList.add('rift-active'); | ||
| statusEl.classList.add('rift-status'); | ||
| render(); | ||
| setStatus('Rift node found. Tap the marked cell.'); | ||
| saveGame(); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
These helper functions are intended only for testing but are included in the main application script. While they are guarded by the TEST_MODE flag, they still add to the production bundle size and increase the application's potential attack surface. For larger applications, it's a best practice to use a build process (like tree-shaking with environment variables) to completely remove such test-specific code from production builds.
| async function openFreshGame(page) { | ||
| await page.addInitScript(() => { | ||
| localStorage.clear(); | ||
| sessionStorage.clear(); | ||
| }); | ||
| await page.goto('/?e2e=1'); | ||
| await expect(page.locator('#board .cell')).toHaveCount(81); | ||
| } |
There was a problem hiding this comment.
This openFreshGame helper function is very useful. Similar setup logic (clearing storage, navigating to the test page) is duplicated in tests/gameplay.placeholder.spec.js and tests/ui.mobile.spec.js. To improve maintainability and reduce code duplication, consider extracting this helper into a shared utility file (e.g., tests/test-utils.js) and importing it where needed across your test suites.
|
@codex review and implement Gemini's code review suggestions |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1ba80a921
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await page.addInitScript(() => { | ||
| localStorage.clear(); | ||
| sessionStorage.clear(); | ||
| }); |
There was a problem hiding this comment.
openFreshGame uses page.addInitScript to clear storage, but Playwright runs init scripts before every navigation in that page, including page.reload(). In the "saved game can be resumed" test, the reload therefore clears localStorage right before app boot, so the resume modal cannot appear and the test fails for the wrong reason.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
playwright.config.jswith a localwebServeron port4173, two projects (chromiumandmobile-chrome), and reporters/tracing options.tests/(ui.smoke.spec.js,ui.mobile.spec.js,gameplay.placeholder.spec.js) that exercise loading, selection, entering numbers, conflicts, notes mode, resume flow, and deterministic Rift behavior.?e2e=1is present by addingTEST_MODEandwindow.__shandokuTesthelpers inscript.js, plus helper functionsnormalizeTestBoardandapplyTestBoardState, and reduce the splash delay in test mode..github/workflows/ui-tests.ymlto install Node, Playwright browsers, runnpm run test:ui, and upload artifacts on failure.package.jsonto add@playwright/test,http-server, and scriptstest:serve,test:ui, andtest:ui:headed.playwright-report/andtest-results/to.gitignore.sw.jstoshandoku-wife-edition-v6to reflect assets changes.Testing
npm run test:uion push and pull requests.npm install,npm run test:serve, andnpm run test:ui(ornpm run test:ui:headed).Codex Task