Skip to content

Chore/playwright mode test plan#779

Open
sstidl wants to merge 4 commits intomasterfrom
chore/playwright-mode-test-plan
Open

Chore/playwright mode test plan#779
sstidl wants to merge 4 commits intomasterfrom
chore/playwright-mode-test-plan

Conversation

@sstidl
Copy link
Copy Markdown
Collaborator

@sstidl sstidl commented Apr 12, 2026

No description provided.

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix e2e tests compatibility and reproducibility

🧪 Tests ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Replace Node.js fetch API with native http/https for Node14 compatibility
• Fix backend-testpoint fixture port from default to 8080
• Use npm ci instead of npm install for reproducible builds
• Improve HTTP readiness check with proper socket cleanup
Diagram
flowchart LR
  A["global-setup.js"] -- "Replace fetch with http/https" --> B["Node14 compatible readiness check"]
  C["servers fixtures"] -- "Update port to 8080" --> D["backend-testpoint configuration"]
  E["CI workflows"] -- "npm ci for reproducibility" --> F["Deterministic dependencies"]
Loading

Grey Divider

File Changes

1. tests/e2e/global-setup.js 🐞 Bug fix +16/-2

Replace fetch with native http/https for compatibility

• Added native http/https module imports for Node14 compatibility
• Created isHttpOk function to replace fetch API with native HTTP client
• Implemented proper socket cleanup via res.resume() to prevent resource leaks
• Simplified waitForReady function to use new isHttpOk helper

tests/e2e/global-setup.js


2. .github/workflows/docker-publish.yml ⚙️ Configuration changes +1/-1

Use npm ci for reproducible builds

• Changed npm install to npm ci for reproducible dependency installation
• Ensures exact versions from package-lock.json are used

.github/workflows/docker-publish.yml


3. .github/workflows/playwright.yml ⚙️ Configuration changes +1/-1

Use npm ci for reproducible builds

• Changed npm install to npm ci for reproducible dependency installation
• Ensures exact versions from package-lock.json are used

.github/workflows/playwright.yml


View more (2)
4. tests/e2e/fixtures/servers-dual.json 🐞 Bug fix +1/-1

Fix backend-testpoint port configuration

• Updated backend-testpoint server URL from http://backend-testpoint/ to
 http://backend-testpoint:8080/
• Fixes port mismatch in Docker Compose configuration

tests/e2e/fixtures/servers-dual.json


5. tests/e2e/fixtures/servers-frontend.json 🐞 Bug fix +1/-1

Fix backend-testpoint port configuration

• Updated backend-testpoint server URL from http://backend-testpoint/ to
 http://backend-testpoint:8080/
• Fixes port mismatch in Docker Compose configuration

tests/e2e/fixtures/servers-frontend.json


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects bot commented Apr 12, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. No readiness request timeout🐞
Description
In tests/e2e/global-setup.js, waitForReady() awaits isHttpOk(), but isHttpOk() creates an
http(s).get request without any timeout/abort logic, so a connection that stalls before returning
headers can hang forever and prevent the loop-level timeout from ever firing.
Code

tests/e2e/global-setup.js[R7-27]

+function isHttpOk(url) {
+  return new Promise((resolve, reject) => {
+    const client = url.startsWith('https://') ? https : http;
+    const req = client.get(url, (res) => {
+      const status = res.statusCode || 0;
+      // Drain body so sockets can be reused/closed cleanly.
+      res.resume();
+      resolve(status >= 200 && status < 300);
+    });
+    req.on('error', reject);
+  });
+}
+
async function waitForReady(name, url, timeoutMs) {
 const start = Date.now();
 while (Date.now() - start < timeoutMs) {
   try {
-      const response = await fetch(url, { redirect: 'manual' });
-      if (response.ok) {
+      if (await isHttpOk(url)) {
       return;
     }
   } catch {
Evidence
isHttpOk() wraps http(s).get() in a Promise that only resolves on the response callback or rejects
on the 'error' event; without a request timeout, the Promise may never settle. Since waitForReady()
does await isHttpOk(url) inside its while loop, an unsettled Promise blocks the loop and makes the
timeoutMs check ineffective.

tests/e2e/global-setup.js[7-17]
tests/e2e/global-setup.js[20-33]

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

## Issue description
`isHttpOk()` uses `http(s).get()` without a timeout/abort, so `await isHttpOk(url)` can hang indefinitely and prevent `waitForReady()` from enforcing `timeoutMs`.
### Issue Context
`waitForReady()` is intended to bound startup waits (e.g., `timeoutMs = 180_000`). However, the timeout is only checked between iterations; if a single request never completes, the loop never progresses.
### Fix Focus Areas
- tests/e2e/global-setup.js[7-33]
### Suggested fix
Add a per-request timeout (e.g., 5–10s) and destroy/abort the request on timeout, ensuring the Promise always resolves `false` or rejects promptly. Example approach:
- call `req.setTimeout(10_000, () => req.destroy(new Error('timeout')))`
- optionally also handle `req.on('timeout', ...)`
- keep `res.resume()` to drain the body
This guarantees `waitForReady()` can continue retrying and eventually throw its own timeout error.

ⓘ 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

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.

2 participants