Skip to content

Fix plugin frontend-tests template: use dev mode not prod#7402

Merged
JohnMcLear merged 1 commit into
ether:developfrom
JohnMcLear:fix/frontend-tests-dev-mode
Mar 31, 2026
Merged

Fix plugin frontend-tests template: use dev mode not prod#7402
JohnMcLear merged 1 commit into
ether:developfrom
JohnMcLear:fix/frontend-tests-dev-mode

Conversation

@JohnMcLear

Copy link
Copy Markdown
Member

Summary

  • Change pnpm run prod to pnpm run dev in frontend-tests.yml template
  • Prod mode enables rate limiting which causes plugin frontend tests to fail silently (tests fail but step exits 0 because the server is backgrounded)

Test plan

  • Verified on ep_headings2 — prod mode caused silent test failures

🤖 Generated with Claude Code

Prod mode enables rate limiting which causes frontend tests to fail
silently. Dev mode disables rate limiting for testing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Review Summary by Qodo

Fix plugin frontend-tests template to use dev mode

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Change frontend-tests template from prod to dev mode
• Disables rate limiting that causes silent test failures
• Ensures plugin frontend tests fail visibly when issues occur
Diagram
flowchart LR
  A["frontend-tests.yml"] -- "change pnpm run prod to pnpm run dev" --> B["Dev mode enabled"]
  B -- "disables rate limiting" --> C["Tests fail visibly"]
Loading

Grey Divider

File Changes

1. bin/plugins/lib/frontend-tests.yml 🐞 Bug fix +1/-1

Switch frontend tests from prod to dev mode

• Changed pnpm run prod to pnpm run dev in test execution step
• Disables rate limiting in dev mode to prevent silent test failures
• Ensures plugin frontend tests properly report failures instead of exiting with success code

bin/plugins/lib/frontend-tests.yml


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Mar 31, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. No regression test for dev change 📘 Rule violation ⛯ Reliability
Description
This PR changes the frontend tests workflow to fix a silent-failure bug but does not add any
automated regression check that would fail if the prod command were reintroduced. Without a
regression test/assertion, the original issue can be reintroduced unnoticed.
Code

bin/plugins/lib/frontend-tests.yml[81]

+          pnpm run dev &
Evidence
PR Compliance ID 1 requires a regression test for bug fixes. The diff shows only a workflow command
change from pnpm run prod & to pnpm run dev &, with no accompanying automated assertion/test
added in the changed code to prevent reintroducing the silent failure mode.

bin/plugins/lib/frontend-tests.yml[81-81]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A bug fix was applied to the frontend test workflow (`pnpm run prod` -> `pnpm run dev`), but there is no regression check that would fail if the workflow reverted to the previous behavior (silent failures due to a backgrounded server).

## Issue Context
This workflow runs the plugin frontend tests; the PR description indicates prod mode can cause tests to fail silently while the step exits 0.

## Fix Focus Areas
- bin/plugins/lib/frontend-tests.yml[81-84]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Readiness result ignored 🐞 Bug ⛯ Reliability
Description
The workflow’s connection loop stops after 15 seconds even if Etherpad never becomes reachable, but
the script proceeds to run Playwright anyway because the connected flag is never checked. This can
turn server-start problems into confusing/flaky test failures instead of a clear early exit.
Code

bin/plugins/lib/frontend-tests.yml[R82-84]

          connected=false
          can_connect() {
          curl -sSfo /dev/null http://localhost:9001/ || return 1
Evidence
The script initializes connected=false, sets it to true only inside can_connect() after a
successful curl, but never checks connected after the while-loop before running Playwright.

bin/plugins/lib/frontend-tests.yml[81-94]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow continues to `pnpm run test-ui` even if the server never becomes reachable within the 15s wait loop.

### Issue Context
A `connected` flag is set but never used to gate execution after the wait loop.

### Fix Focus Areas
- bin/plugins/lib/frontend-tests.yml[81-94]

### Suggested change
After the wait loop, add an explicit check that exits non-zero if the server never became reachable (and optionally print a helpful error message). Consider adding a small curl timeout (e.g., `--connect-timeout`/`--max-time`) so the health check can’t hang indefinitely.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Unmanaged background server 🐞 Bug ⛯ Reliability
Description
pnpm run dev is started in the background without capturing its PID, monitoring if it exits early,
or killing it on script exit. This can hide server startup crashes (the step doesn’t fail at the
moment the server dies) and can leave a stray server process running for the rest of the job.
Code

bin/plugins/lib/frontend-tests.yml[81]

+          pnpm run dev &
Evidence
The server is launched with & and the script has no PID tracking ($!), no trap cleanup, and no
liveness check before proceeding to run tests.

bin/plugins/lib/frontend-tests.yml[81-94]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The backgrounded Etherpad server process is not tracked, cleaned up, or monitored for early termination.

### Issue Context
The workflow starts the server with `&` then runs other commands; if the server process dies, the script won’t notice immediately, and the process isn’t killed on exit.

### Fix Focus Areas
- bin/plugins/lib/frontend-tests.yml[81-94]

### Suggested change
Capture the PID (e.g., `server_pid=$!`), add a `trap` to `kill` it on exit, and optionally check `kill -0 "$server_pid"` inside the wait loop to fail fast if the server process exits before becoming reachable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@JohnMcLear JohnMcLear merged commit a6d283a into ether:develop Mar 31, 2026
20 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant