Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion frontend/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export default defineConfig({
testDir: './e2e/tests',
testMatch: '**/*.spec.ts',
forbidOnly: isCI,
globalTimeout: parseInt(process.env.GLOBAL_TIMEOUT_MS || '0', 10) || (isCI ? 110 * 60 * 1000 : 0),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant config file with line numbers.
sed -n '1,160p' frontend/playwright.config.ts | cat -n

Repository: openshift/console

Length of output: 5460


🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '1,160p' frontend/playwright.config.ts | cat -n

Repository: openshift/console

Length of output: 5460


🏁 Script executed:

#!/bin/bash
set -euo pipefail
pwd
ls

Repository: openshift/console

Length of output: 844


🌐 Web query:

Playwright globalTimeout 0 meaning disable timeout documentation

💡 Result:

In Playwright, setting globalTimeout to 0 means that the timeout behavior is disabled, allowing the test suite to run for an unlimited amount of time [1][2]. The globalTimeout configuration option defines the maximum time in milliseconds that the entire test suite is allowed to run [1]. By default, there is no global timeout (i.e., it is unlimited) [3][4]. When you explicitly set globalTimeout to 0, you are effectively choosing to disable any automatic termination of the test run based on duration, which can be useful if you do not want Playwright to stop the process prematurely [1][2]. This is distinct from other timeout settings in Playwright, where specific values are used to enforce constraints on individual tests, actions, or hooks to prevent resource wastage [3][4].

Citations:


Honor an explicit zero timeout override.

GLOBAL_TIMEOUT_MS=0 is treated as unset in CI because of ||, so the 110-minute default still applies. Check for an explicit env value before falling back.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/playwright.config.ts` at line 52, Update the globalTimeout
configuration expression to distinguish an explicitly provided GLOBAL_TIMEOUT_MS
value of "0" from an unset or invalid value. Use the parsed environment value
whenever GLOBAL_TIMEOUT_MS is explicitly present, and only apply the isCI-based
110-minute default when the variable is absent.

Source: Path instructions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ternary/or combination is somewhat complex, maybe GLOBAL_TIMEOUT_MS can be set in the test-prow-playwright-e2e script instead?

retries: isCI ? 1 : 0,
timeout: 120_000,
reporter: isCI
Expand Down Expand Up @@ -84,7 +85,7 @@ export default defineConfig({
},
},

workers: process.env.WORKERS ? parseInt(process.env.WORKERS, 10) : isCI ? 1 : undefined,
workers: process.env.WORKERS ? parseInt(process.env.WORKERS, 10) : undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail
rg -n 'WORKERS|workers' frontend test-prow-playwright-e2e.sh
rg -n '"`@playwright/test`"|`@playwright/test`@' package.json yarn.lock frontend/package.json frontend/yarn.lock 2>/dev/null || true

Repository: openshift/console

Length of output: 694


Preserve WORKERS syntax instead of coercing it with parseInt.
parseInt accepts malformed values like 2foo and strips supported Playwright percentage forms such as 50%. Validate the env var first, or pass through a valid worker string unchanged.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/playwright.config.ts` at line 88, Update the workers configuration
to preserve valid WORKERS values, including Playwright percentage strings,
instead of coercing them with parseInt. Validate the environment value before
use and pass the original string through unchanged when valid, while retaining
undefined when WORKERS is unset and rejecting malformed values such as “2foo”.

Source: Path instructions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since we are not supporting percentage values we can be stricter and use Number() that is stricter than parseInt()

Suggested change
workers: process.env.WORKERS ? parseInt(process.env.WORKERS, 10) : undefined,
workers: process.env.WORKERS ? Number(process.env.WORKERS) || undefined : undefined,


projects: [
{
Expand Down
2 changes: 2 additions & 0 deletions test-prow-playwright-e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ export BRIDGE_BASE_ADDRESS="$(oc get consoles.config.openshift.io cluster -o jso

./contrib/create-user.sh

export WORKERS="${WORKERS:-2}"

pushd frontend

SCENARIO="${1:-e2e}"
Expand Down