ci: execute all frontend and contract tests#832
Conversation
PR Reviewer Guide 🔍(Review updated until commit c43b1d9)Here are some key observations to aid the review process:
|
appergb
left a comment
There was a problem hiding this comment.
Independent senior re-review — logical verdict: REQUEST_CHANGES
Reviewed exact head 567f7ca70d5a62c3ab87dd8035032940d26b0804 against current beta 472b914985ec525bb082582fd7968a43c7d9ce41 (PR merge-base / recorded base 9991b325b8ea035ab167d486c5ba3c71688418a9). Submitted as COMMENTED because the authenticated account is also the PR author.
Strengths and confirmed behavior
- Independent tracked-file enumeration and
discoverTestFiles()produce the same sorted set of 19 checks: allsrc/**/*.test.ts, allscripts/**/*.test.mjs, and all rootscripts/check-*.mjs. The final default sort plus separator normalization gives stable slash-separated relative paths. - TypeScript tests execute with the pinned local
tsx@4.23.1CLI throughprocess.execPath;.mjschecks execute directly through the same Node binary. There is nonpx, shell chaining, or runtime download path. - Node 22 behavior is exercised locally (
v22.17.1) and in CI; Windows usedv22.23.1. The production runner's native absolute path resolution is correct on Windows. - From a clean worktree with recursively initialized
qwen-asr, cleannpm ci, nodist, and a new Cargo target,npm testcorrectly ranpretest -> prebuild contract -> tsc/vite build -> 19 checks. The macOS/Linux hotkey gate compiled and ranhotkey_injection_gate_logs_pressed_and_cancels; the generateddist/index.htmlexisted before Cargo consumed TaurifrontendDist. - The Windows hotkey early-success path is consistent with the later
cargo test --lib --no-runcompile gate and the separate Rust-only backend suite. The aggregate workflow still includes the prior build, macOS capsule contract, Windows UI contract, preflight, PowerShell checks, Cargo gates, and version-sync gate. - The injected self-test does verify exact helper status
7propagation and fail-fast after two calls. More importantly, the exact-head Windows run supplies end-to-end evidence that a real child failure becomes the aggregatenpm testexit status: the job stops immediately at that child and subsequent steps are skipped. - Scope is limited to the test harness/workflow/package scripts and Windows hotkey gate; no product or UI behavior changed.
Critical
- None.
Important / required
-
The new self-test is not cross-platform and makes the required Windows gate fail.
File:
openless-all/app/scripts/frontend-test-runner.test.mjs:40-41The test passes
appRoot: '/app'but hard-codes expected child paths as POSIX strings. On Windows, the production runner correctly resolves those paths to the current drive, so exact-head CI reports:- actual:
D:\app\src\lib\passes.test.ts - expected:
/app/src/lib/passes.test.ts
The same defect also applies to the expected failing
.mjspath on line 41. Because the self-test is itself discovered by the aggregate runner, Windows fails atBuild frontend and run frontend/contract tests;cargo check,cargo test --lib --no-run, the Rust-only Windows suite, and version-sync verification are all skipped. This directly violates #806's cross-platform acceptance criterion and leaves only four of the five exact-head checks green.Build the expected paths with
node:path(resolve/join) using the same platform-native semantics as the runner, then rerun the exact-head Windows job and confirm all later Windows gates execute successfully. - actual:
Minor
- None.
Independent verification
- Clean
npm ci: 384 packages installed; 0 vulnerabilities. - Empty-dist
npm test: passed locally; 19 discovered checks, including the hotkey Cargo gate and all tracked frontend/contracts. npm audit --audit-level=high: 0 vulnerabilities.node --checkfor all three changed.mjsfiles: passed.ci.ymlYAML parse,git diff --check, and tracked-vs-discovered set comparison: passed.- Exact-commit gitleaks scan (
9991b325...567f7ca7): no leaks. - Current-beta merge-tree: clean (
15a61140519e4e0ddac05b06d79a9345f63da146), with all five changed-file blobs identical to exact head. - Exact-head checks: Android, macOS, Linux, and PR-Agent succeeded; Windows failed in
frontend-test-runner.test.mjsas described above. - Final drift check: PR head remains
567f7ca70d5a62c3ab87dd8035032940d26b0804; remotebetaremains472b914985ec525bb082582fd7968a43c7d9ce41. The PR is still Draft.
Ready-to-merge verdict
Not ready to merge. The aggregate runner design and coverage are otherwise sound, but its own platform-specific assertion currently breaks the mandatory Windows quality gate and prevents the downstream Windows checks from running.
567f7ca to
3c33578
Compare
|
Addressed independent review blocker at exact head
Fresh exact-head CI and an independent re-review are required before merge. |
|
Persistent review updated to latest commit 3c33578 |
appergb
left a comment
There was a problem hiding this comment.
Logical verdict: APPROVE
Re-reviewed exact head 3c335783898ea8c303d6a0a9b66a990ce2e5a5fe against current beta e3e63cad040ff4bd5eefc439d9be065f2b4f8a43.
Findings:
- Critical: none.
- Important: none.
- Minor: none.
The previous Windows blocker is closed. The self-test now builds expected child paths with node:path.resolve, so the assertions follow native Windows path semantics instead of comparing D:\\app\\... against hard-coded POSIX paths. The exact-head Windows log confirms the aggregate step passed and reported all 21 tests.
Independent verification:
- Scope is limited to five CI/test-harness files; no product UI, runtime UI, or user-facing source paths change.
- Aggregate discovery exactly matches all 21 tracked
src/**/*.test.ts,scripts/**/*.test.mjs, andscripts/check-*.mjsfiles, with no omissions or duplicates. This includes the mergedpin-persistence-security-contract.test.mjsandgithubOauth.test.tscoverage. - A fresh isolated checkout with no pre-existing
distpassednpm ciandnpm test: pretest build, the real Rust hotkey injection gate, all 21 aggregate children, the PIN contract, and the runner self-test all completed successfully. - The self-test still proves exact failure propagation and fail-fast behavior with synthetic statuses
[0, 7, 0]: status 7 is returned and only the first two children are invoked. TypeScript tests use the repository-pinnedtsx@4.23.1CLI throughprocess.execPath; nonpxor runtime download path is introduced. - Exact-head Windows CI logged 21 discovered tests, the expected Windows-only hotkey runtime skip, the aggregate self-test pass, and the PIN contract pass; it then also passed
cargo check, Windows lib-test compilation, actual Rust-only backend tests, and version synchronization. - Clean merge-tree generation produced
b41bad44a5d17cf63183a62ad680d800902fdd5d, byte-equivalent to the exact head, with only the five reviewed paths. npm audit --audit-level=highreports 0 vulnerabilities; Node syntax checks, workflow YAML parsing,git diff --check, and gitleaks all pass.- All five exact-head checks are green: PR Agent, Android, macOS, Windows, and Linux.
Given this evidence, no blocking or non-blocking correctness finding remains.
3c33578 to
5eaa030
Compare
|
Rebased the aggregate test gate after #825 merged. Exact head is now The workflow conflict was resolved by retaining the single aggregate Fresh exact-head five-platform CI and another independent exact-head review are required before merge. |
|
Persistent review updated to latest commit 5eaa030 |
appergb
left a comment
There was a problem hiding this comment.
Logical verdict: REQUEST_CHANGES (submitted as a COMMENTED review).
Re-reviewed exact head 5eaa030d0c71d30c430e2e5293b110d64689b21e against current beta eb099c4f411af546628408e30482d23b2fcc6bf8. The head is a direct child of that beta. git merge-tree --write-tree is conflict-free and returns 1eb88dc99ac52e5595d6f9a2a05f25c0ecd8df6a, exactly the head tree.
Important / required
-
[P1] Preserve the macOS-only contract semantics in the aggregate cross-platform step.
discoverTestFiles()unconditionally includes everyscripts/**/*.test.mjsfile (frontend-test-runner.mjs:23-32), while the replacement workflow now runsnpm testunconditionally on macOS, Windows, and Linux (ci.yml:155-156). Current beta addedmacos-speech-usage-description-contract.test.mjs; that contract invokesbashand the checker hard-codes/usr/bin/plutil. Before this PR, the workflow guarded this test withif: runner.os == 'macOS'; the aggregate replacement removes that platform condition without adding equivalent runner/test gating.Exact-head CI proves the failure on both non-macOS platforms:
- Windows discovers all 22 tests, passes the runner self-test and native
node:path.resolveassertions, then the macOS speech contract fails with status 127:/usr/bin/plutil: No such file or directory. The aggregate step exits 1. - Linux likewise discovers 22, reaches the same contract, receives status 127 for
/usr/bin/plutil, and exits 1. - Because the runner correctly fail-fasts, all later aggregate children plus
cargo check, the Windows compile/runtime Rust gates, and version-sync verification are skipped on those jobs.
This is specifically a workflow-replacement regression, not a Windows path regression. Please keep discovery at the expected 22 tracked checks but preserve platform applicability—for example, make the macOS contract explicitly skip on non-Darwin systems or add a tested platform-aware runner mapping—then rerun exact-head Windows and Linux through every downstream gate.
- Windows discovers all 22 tests, passes the runner self-test and native
Confirmed behavior
- Independent tracked-file enumeration exactly matches aggregate discovery: 22 tracked / 22 discovered, no missing files, extras, or duplicates. The list includes
macos-speech-usage-description-contract.test.mjs. - The prior Windows path bug is fixed: self-test expectations use
node:path.resolve, and exact-head Windows passesfrontend-test-runner.test.mjsbefore hitting the unrelated macOS-only contract. - The self-test still proves fail-fast and exact child status propagation: injected statuses
[0, 7, 0]return 7 and invoke only the first two children. - TypeScript tests execute via
process.execPathplus the pinned localtsx@4.23.1CLI;.mjstests execute through the same Node binary. No shell evaluation,npx, or runtime download path is introduced. - A fresh independent worktree with recursively initialized submodules, no
dist, no Cargo target, and cleannpm cipassed local macOSnpm test: pretest builtdist, then all 22 children passed. The full Rust library suite also passed: 701 passed, 0 failed. - Scope remains five CI/test-harness files. There are no UI, styling, component, or user-facing runtime changes.
Verification
npm audit --audit-level=high: 0 vulnerabilities.cargo audit: exit 0 with 18 allowed warnings.- Exact-commit gitleaks: 1 commit / ~5.05 KB scanned, no leaks.
- Changed
.mjssyntax checks, workflow YAML parse, andgit diff --check: passed. - GitNexus compare: 5 files / 18 symbols, 0 affected product execution flows, low risk.
- Exact-head remote checks: Android cargo check, macOS checks, and PR-Agent pass; Windows and Linux fail at the macOS speech contract described above.
No other correctness or scope finding remains, but the two mandatory cross-platform jobs must be green before approval.
5eaa030 to
c43b1d9
Compare
|
Remote CI on 5eaa030 correctly exposed an integration gap: the aggregate runner discovered the newly merged macOS speech plist contract on Linux/Windows, where |
|
Persistent review updated to latest commit c43b1d9 |
appergb
left a comment
There was a problem hiding this comment.
Logical verdict: APPROVE (submitted as a COMMENTED review).
Fresh re-review of exact head c43b1d96e48650bac1583b8902cfc3c326d92cd2 against current beta eb099c4f411af546628408e30482d23b2fcc6bf8.
Findings:
- Critical: none.
- Important: none.
- Minor: none.
The previous exact-head blocker is closed without masking macOS coverage:
- Independent tracked-file enumeration exactly matches aggregate discovery: 22 tracked / 22 discovered, no missing files, extras, or duplicates. The list and the runner self-test explicitly include
scripts/macos-speech-usage-description-contract.test.mjs. - The contract now exits successfully before invoking
bash//usr/bin/plutilonly whenprocess.platform !== 'darwin'. Exact-head Windows and Linux logs each show 22 discovered tests, the explicit non-macOS speech skip, and immediate continuation into the PIN contract and every later aggregate child. - Exact-head macOS logs show the speech contract executing between the capsule and PIN contracts with no skip message. An independent local assertion on the macOS review host also ran the contract with
{ platform: "darwin", status: 0, stdout: "", stderr: "", skipped: false }, so the new guard does not bypass the real macOS validation.
Independent verification:
- The head is a direct child of current beta.
git merge-tree --write-treeis conflict-free and returns84cb849f75497255105e93d0015ac79360cb8d06, exactly the head tree. - A fresh detached worktree started with no
node_modules, nodist, and no Cargo target. After recursive submodule initialization and cleannpm ci,npm testbuilt the frontend first and then passed all 22 aggregate children, including the real hotkey Cargo gate and real macOS speech contract. - The runner self-test proves fail-fast and exact child status propagation: injected statuses
[0, 7, 0]return 7 and invoke only the first two children. Windows-native path expectations usenode:path.resolve; exact-head Windows passes the self-test. - TypeScript tests use
process.execPathwith the repository-pinnedtsx@4.23.1CLI..mjschecks use the same Node executable; there is no shell evaluation,npx, or runtime download path. - Full local Rust library suite: 701 passed, 0 failed. Exact-head macOS also passes 701; Linux passes its 687 cfg-specific tests; Windows compiles the full lib-test binary and passes all 130 Rust-only backend tests.
- Workflow replacement semantics are preserved: macOS, Linux, and Windows run the aggregate step, then their Cargo and version-sync gates. The Android job retains its separate build/updater/target-check path.
- Scope is limited to six CI/test-harness files. No product UI, component, style, or user-facing runtime source changed.
- Changed
.mjssyntax checks, workflow YAML parse, andgit diff --check: passed. npm audit --audit-level=high: 0 vulnerabilities.cargo audit: exit 0 with 18 allowed warnings. Exact-commit gitleaks: no leaks.- GitNexus compare: 6 files / 19 symbols, 0 affected product execution flows, low risk.
- All five exact-head checks are green: Android cargo check, macOS checks, Windows checks, Linux checks, and PR-Agent.
Final drift check immediately before this review: head remains c43b1d96e48650bac1583b8902cfc3c326d92cd2; beta remains eb099c4f411af546628408e30482d23b2fcc6bf8. No correctness or scope finding remains.
|
Persistent review updated to latest commit c43b1d9 |
User description
Closes #806
Summary
npm testentry point that discovers every trackedsrc/**/*.test.ts,scripts/**/*.test.mjs, andscripts/check-*.mjschecktsxCLI through Node withoutnpx, shell chaining, or runtime downloadspretestso the Tauri hotkey gate has a cleanfrontendDistnpm ciRED
npm testfailed because no aggregate runner existedcheck-*.mjs, proving those checks would otherwise be omitteddist, the hotkey child failed at TaurifrontendDist, leading to the deterministicpretestbuildGREEN / verification
CARGO_TARGET_DIR=/Volumes/mac/openless-target/issue-806 npm test(19 discovered checks; all passed)npm audit --audit-level=high(0 vulnerabilities).mjsfilesgit diff --checkNo product or UI behavior changes.
PR Type
Enhancement, Tests
Description
Replace multiple CI steps with a single
npm testcommandAdd aggregate test runner that discovers all test files
Add platform-specific skips for hotkey and speech tests
Diagram Walkthrough
File Walkthrough
ci.yml
Consolidate CI test steps into one.github/workflows/ci.yml
npm testcommand.contracts.
package.json
Add test scripts to package.jsonopenless-all/app/package.json
pretestscript to build frontend before testing.testscript to invoke the aggregate test runner.frontend-test-runner.mjs
Aggregate test runner implementationopenless-all/app/scripts/frontend-test-runner.mjs
.test.ts,.test.mjs,check-*.mjs).discoverTestFilesandrunTestFilesfor reuse.frontend-test-runner.test.mjs
Self-test for aggregate runneropenless-all/app/scripts/frontend-test-runner.test.mjs
propagation.
check-hotkey-injection.mjs
Skip hotkey runtime gate on Windowsopenless-all/app/scripts/check-hotkey-injection.mjs
cargo test --no-run.macos-speech-usage-description-contract.test.mjs
Skip speech contract on non-macOSopenless-all/app/scripts/macos-speech-usage-description-contract.test.mjs