Automation tests CI/CD integration - #442
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #442 +/- ##
==========================================
+ Coverage 75.01% 75.27% +0.26%
==========================================
Files 524 526 +2
Lines 35301 35423 +122
==========================================
+ Hits 26480 26664 +184
+ Misses 8821 8759 -62
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Thanks for this — the suite itself is in good shape. I ran the full thing locally against this branch: 16/16 tests passed in 7.3 minutes (Playwright 1.60.0/Chromium 148, which is what the committed lockfile actually resolves to), including both full stack teardown/rebuilds. The journey coverage (install → registrants → groups → areas → approval workflow → vocabulary codes) is real and working. Review verdict is changes requested on one blocker plus a few workflow items.
Must fix before merge
The fixture PDFs in e2e/fixtures/ contain what appears to be real personal data. Three of the five look like internet-sourced sample documents of real, identifiable people: sample_BIR_2316.pdf is a filled and signed tax certificate (names, TINs, home address, birth date, salary figures), sample_authorization_letter.pdf names two specific individuals, and sample_cert_of_enrolment.pdf names a minor along with their real school and principal. For a social-protection platform whose core mission is protecting PII, we cannot commit these into the public repo's permanent history. (sample_valid_ID_parent.pdf — a cartoon mock-up — and sample_academic_calendar.pdf are fine.)
Note that no test currently uploads these files (there is no setInputFiles in the suite — tests 14–15 only create vocabulary codes named after them), so this costs nothing functionally:
- Replace the three PDFs with self-made fabricated documents (obviously fictional names like Juan Dela Cruz, invalid TIN patterns, fictional school).
- Rewrite the branch history (interactive rebase or fresh branch + force-push) so the original files never existed in any commit that lands on
19.0— replacing them in a new commit is not enough, git history keeps the originals forever.
Should fix before merge
- The workflow's Start OpenSPP stack / Wait for healthy steps are wasted work: each spec's
beforeAllcallsresetStack(), which immediately doesdown -vand rebuilds. Drop the workflow steps (keepresetStack), or skipresetStackwhen CI already provides a fresh stack. (Also meansODOO_URLcan't actually point anywhere remote, sincehelpers.tsshells out to localdocker compose.) - Use
npm ciand cache on the lockfile so CI installs exactly what's locked. e2e/Dockerfileis referenced by nothing (the workflow installs Node directly, helpers use compose) and pinsv1.44.0-jammywhile the lockfile resolves 1.60.0. Either wire it up or delete it.- Guard the Discord step so a missing/unset
DISCORD_WEBHOOKsecret doesn't fail thenotifyjob (e.g.if: ${{ secrets.DISCORD_WEBHOOK != '' }}via an env indirection, or|| truewith a warning). - Rename
e2e/CLAUDE.mdtoe2e/README.md— its content is a README, and this repo doesn't commit CLAUDE.md files.
Non-blocking observations
- Heads-up that this workflow has never executed on an actual GitHub runner (it only triggers on push to
19.0, andworkflow_dispatchneeds the file on the default branch) — the first real run happens after merge. My local 7.3m run had a warm Docker build cache; a cold runner at ~5x slower compute will land somewhere in the 25–45 min range per matrix job, inside the 60-min timeout but worth watching on the shakedown run. - See inline comments for the
action-178regex, the 30s hard sleep, andslowMo.
Happy to re-review as soon as the fixtures are replaced and the history is rewritten — that part is the only hard gate.
COMPOSE_CWD pointed at /home/penn_ubuntu/OpenSPP2, which only existed on the machine that authored the test. Anywhere else (other devs, CI) it failed with a misleading "spawnSync /bin/sh ENOENT" since Node reports a missing cwd as a shell-not-found error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes the pre-commit prettier hook failure from CI on these files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Drop the "Start OpenSPP stack"/"Wait for healthy" steps: each spec's beforeAll already calls resetStack(), which tears down and rebuilds the stack from scratch, making the workflow steps wasted work. - Cache dependencies on package-lock.json instead of package.json, and use npm ci instead of npm install so CI installs exactly what's locked. - Guard the Discord notify step so a missing DISCORD_WEBHOOK secret skips the notification instead of failing the job. - Bump e2e/Dockerfile's base image from v1.44.0-jammy to v1.60.0-jammy to match what package-lock.json actually resolves for @playwright/test. (It is used, via the repo's root docker-compose.yml e2e-runner service under the "e2e" profile — not unused as originally assumed.) Addresses PR review feedback from gonzalesedwin1123.
- Drop the action-178 URL regex wait: it depends on a DB-assigned action id that renumbers whenever install order or seed data changes. The existing wait for the nav menu entry to become visible already covers "installation finished", so the URL assertion was redundant and fragile. - Replace the fixed 30s sleep in test 01 with polling for the Registry menuitem to become visible, which is the actual condition being waited on. - Make Playwright's slowMo opt-in (PWDEBUG_SLOWMO) instead of a global 500ms delay on every action, including headless CI runs. Addresses PR review feedback from gonzalesedwin1123.
resetStack() assumed it always runs on a host with Docker installed and Odoo reachable at localhost. Neither holds inside the e2e-runner service (docker-compose.yml, "e2e" profile): it has no Docker CLI/socket to run `docker compose` with, and "localhost" there refers to the container itself, not the openspp service. - Read the health-check URL from ODOO_URL (falls back to localhost for the existing host-based flow), instead of hardcoding localhost. - Skip the docker teardown/rebuild entirely when E2E_SKIP_STACK_RESET=true, since openspp's own `depends_on: condition: service_healthy` already guarantees a fresh, healthy stack before e2e-runner starts. - Set E2E_SKIP_STACK_RESET=true on the e2e-runner service in docker-compose.yml, and document the containerized flow in e2e/README.md. Neither change affects the GitHub Actions workflow: it never sets E2E_SKIP_STACK_RESET, and it already sets ODOO_URL=http://localhost:8069 explicitly, so resetStack() behaves exactly as before there.
4ee725d to
5c0df8d
Compare
|
Thanks for the thorough review @gonzalesedwin1123! Fixed everything — fixture PDFs replaced with fabricated ones and branch history rewritten (51b0ca8 → a2bb476, force-pushed), plus all the workflow/spec fixes below. Replied + resolved each item inline. Ready for re-review whenever you get a chance. |
Fixes what CI's pre-commit prettier hook flagged: quote style in .github/workflows/e2e.yml and line-wrap width in the new e2e-runner section of e2e/README.md.
Not a timing issue: Odoo renders "Registry" as a menuitem in two
places once spp_starter_sp_mis is loaded (top navbar dropdown, side
nav list), so getByRole('menuitem', {name: 'Registry'}) threw a strict
mode violation ("resolved to 2 elements") rather than actually timing
out — it just took the full timeout window to report that, which
initially looked like a timing problem.
Fixed by using .nth(1), matching the same disambiguation test 02
already uses when it clicks this same menuitem. Kept the 120s ceiling
since it's still a condition-based poll — costs nothing once install
genuinely finishes faster.
Reverts the earlier opt-in change (PWDEBUG_SLOWMO). Validated with 5
consecutive clean runs of 01-spp-starter-spmis.spec.ts with the delay
in place, versus repeated failures without it, all on identical code.
Removing the per-action delay doesn't just speed up the suite — it
reliably exposes 3 distinct races in Odoo's own OWL frontend that no
human interaction speed would ever trigger:
1. Navbar "Configuration" button ambiguity when switching between apps
that each have a same-named top-level Configuration menu
(spp_approval's menu_approval_config vs.
spp_change_request_v2's menu_change_request_config) — OWL's keyed
diff can leave both apps' buttons mounted mid-transition.
2. Vocabulary "Add a line" row-insertion race, where filling a row via
.o_data_row.last() immediately after adding it can scramble which
code/display pair lands in which row.
3. Odoo's webclient occasionally restores current_action/menu_id from
sessionStorage on a fresh login instead of waiting for the server's
real default-action response, landing on a stale page.
Documented in e2e/README.md ("Known frontend timing races") with file
references, and in a config comment, so this isn't silently reverted
again without addressing the underlying races directly.
Also adds a test.afterEach hook to both spec files that pauses (opens
the Inspector) on a failed test when running locally, so a real
failure can be inspected live instead of the browser closing
immediately. Gated on !process.env.CI (and the e2e-runner container
defaults CI=true) so it never hangs an automated run.
Why is this change needed?
Integrate GUI automation tests in CI/CD via github actions
How was the change implemented?
separate e2e folder, to make sure it does not affect other files
New unit tests
Unit tests executed by the author
How to test manually
run local openspp via docker compose
cd into e2e, npx playwright test --headed
Related links
.github/workflows/e2e.yml