[johallar]

💬 Description

Mostly opening just to test new puppeteer copy-pr-bot stuff

[simoneves]

Claude has lots to say

---

I reviewed the PR as it stands: your local playwright-ci-automation branch is at 8ce390a, the same commit as the head of PR #27, with the same three commits on top of main. The approach is sound: one Playwright runner package, run against the same disposable HeavyDB and Immerse build that Puppeteer uses, with datasets restored from S3. But I'd expect the Playwright step to fail as written. I read the code but didn't run anything, since gh isn't installed here and I can't see the CI results.

Likely to break the run

1. colors.ui.test.ts imports @material-ui/core, which won't be installed. The Playwright step points the repo-root node_modules at test-config/playwright/node_modules with a symlink, and that folder only contains @playwright/test, playwright and playwright-core. Playwright loads every test file before it runs anything, so this missing module either fails the whole run or at least fails that file.

import { hexToRgb } from "@material-ui/core"

It's only used for a colour conversion, so a small local hexToRgb helper is probably easier than adding Material UI to the runner package.

2. nl-to-answer.ui.test.ts needs a working HeavyIQ language-model backend. The tests expect text like "top 5 deepest dams are" from a real natural-language answer. start-heavydb.sh doesn't configure any HeavyIQ or language-model service, and the restore only creates the heavyiq database and its world_dams table. sql-to-answer.ui.test.ts should be fine because it only runs plain SQL against world_dams. The natural-language suite probably needs a test.skip with a reason, like the one used for parameter-url.

Workflow issues

3. Playwright never runs if Puppeteer fails. "Run Playwright test suite" has no if:, so the default "only if earlier steps passed" applies. If both suites are meant to report on every run, use if: ${{ !cancelled() && steps.<restore-step-id>.outcome == 'success' }}. That also means giving the restore step an id.

4. Changes to playwright.config.ts won't trigger the workflow. The on.push.paths filter covers src/** and test-config/**, but the config file sits at the repo root. Add "playwright.config.ts" to the list.

5. The time budget is tight. The job now allows 90 minutes. The steps can take up to 15 for HeavyDB startup, 30 for the restore, 44 for Puppeteer and 30 for Playwright, plus setup. A slow restore plus a slow Puppeteer run will hit the job limit partway through Playwright, which gives a confusing result. Either trim the step limits or raise the job limit.

6. Retries and parallel workers are now active, and both may hide problems. GitHub sets CI=true, so the config's CI settings kick in: 2 retries, 3 parallel workers, and the HTML reporter. Retries can hide flaky tests. Three workers sharing one HeavyDB instance is fine for read-only tests, but watch tests that create or delete dashboards and join data sources (for example test-join-1). Running several copies of the same test can create naming collisions.

Smaller points

7. Use a relative import in utils/charts.ts. Moving off the ui-tests/utils index file is correct, because that index pulls in the Puppeteer helpers. But "ui-tests/utils/common" relies on Playwright honouring the tsconfig baseUrl. The sibling playwright/utils/dashboard.ts already uses "../../utils/common", so matching that is more robust.

8. Consider a conditional skip for parameter-url. The describe.skip turns this suite off for local runs against the shared server too. Something like (process.env.IMMERSE_TEST_URL ? describe.skip : describe) would keep it running locally. That's the same signal sessionization.ui.test.ts uses to detect CI.

9. Naming. The workflow, job, artifacts, scripts folder and HeavyDB container are all still called "puppeteer", and so is the "Restore Puppeteer test data" step. A rename can wait for a follow-up, but it would help.

10. Deleting playwright.yml removes the manual way to run Playwright against the shared test server. That workflow was already obsolete (Node 12/14 and actions/*@v3), so this is fine. Just confirm nobody still relies on it.

What looks good

• The restore script refactor (render_restore_sql, separate heavyai and heavyiq commands, one cleanup of the temp files) is clean. The credentials still stay in 0600 temp files instead of on the command line.
• The new schema checks for the hurricane and dams tables match the columns the tests actually query.
• Pinning @playwright/test at 1.61.1 matches the root package-lock.json, so local and CI runs use the same browser build.
• Letting playwright install --with-deps install the operating-system libraries for both browsers removes the earlier sudo call from the Puppeteer install. Checking that Chrome launches still happens afterwards.
• The us_states → us_states_geo fix in joins-crossfilter lines up with the restored table and its STUSPS join key.

I'd fix the first four before merging. The rest can be follow-ups.