fix: reject workers=0 and negative values with clear error (#39938)#39940
fix: reject workers=0 and negative values with clear error (#39938)#39940Vitalcheffe wants to merge 3 commits intomicrosoft:mainfrom
Conversation
resolveWorkers() previously accepted 0 and negative numbers, causing tests to silently skip with exit code 0. Now throws a descriptive error for any workers value below 1, matching the behavior of forbidOnly (false-positives are caught early). Fixes microsoft#39938
There was a problem hiding this comment.
Pull request overview
This PR prevents false-positive Playwright test runs by rejecting --workers=0 (and other < 1 values) during config resolution, instead of silently running with zero workers.
Changes:
- Added validation in
resolveWorkers()to throw when parsed numeric worker counts are< 1. - Added validation in
resolveWorkers()to throw when numeric worker counts are< 1.
| throw new Error(`Workers must be a positive number, received ${parsedWorkers}.`); | ||
| return parsedWorkers; | ||
| } | ||
| if (workers < 1) | ||
| throw new Error(`Workers must be a positive number, received ${workers}.`); |
There was a problem hiding this comment.
For numeric inputs, workers < 1 doesn’t reject NaN/Infinity or non-integers (e.g. 1.5), which can cause 0 workers or an infinite allocation loop; consider enforcing a finite positive integer (and update the message to reflect “integer/at least 1”).
| throw new Error(`Workers must be a positive number, received ${parsedWorkers}.`); | |
| return parsedWorkers; | |
| } | |
| if (workers < 1) | |
| throw new Error(`Workers must be a positive number, received ${workers}.`); | |
| throw new Error(`Workers must be a positive integer, received ${parsedWorkers}.`); | |
| return parsedWorkers; | |
| } | |
| if (!Number.isFinite(workers) || !Number.isInteger(workers) || workers < 1) | |
| throw new Error(`Workers must be a finite positive integer, received ${workers}.`); |
| if (parsedWorkers < 1) | ||
| throw new Error(`Workers must be a positive number, received ${parsedWorkers}.`); | ||
| return parsedWorkers; | ||
| } | ||
| if (workers < 1) | ||
| throw new Error(`Workers must be a positive number, received ${workers}.`); |
There was a problem hiding this comment.
There are existing config validation tests in tests/playwright-test/config.spec.ts for workers; this change should add coverage for --workers=0/negative values (and ideally invalid percent strings) to prevent regressions of the “silent green run” behavior.
| const parsedWorkers = parseInt(workers, 10); | ||
| if (isNaN(parsedWorkers)) | ||
| throw new Error(`Workers ${workers} must be a number or percentage.`); | ||
| if (parsedWorkers < 1) | ||
| throw new Error(`Workers must be a positive number, received ${parsedWorkers}.`); |
There was a problem hiding this comment.
The '%' branch can still return NaN for inputs like "abc%" (parseInt => NaN), which would make the dispatcher allocate 0 workers and silently skip tests; please validate the parsed percentage and throw a descriptive error when it’s not a finite number.
Skn0tt
left a comment
Skn0tt
left a comment
There was a problem hiding this comment.
Change looks alright, please add tests.
@microsoft-github-policy-service agree |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@Vitalcheffe tests are failing |
|
Fixed — the test assertions were matching the wrong error message path. Config file tests now match the configLoader validation message ( |
Test results for "MCP"1 failed 6295 passed, 360 skipped Merge workflow run. |
Test results for "tests 1"5 flaky39129 passed, 846 skipped Merge workflow run. |
|
I wonder why this block does not trigger in this case? |
That block in configLoader.ts validates the config after it’s been merged and typed — it checks the resolved config shape (e.g. forbidOnly, retries) but doesn’t enforce a positive integer constraint on workers specifically. The resolveWorkers() call happens earlier in the pipeline and returns the raw numeric value directly into the dispatcher, bypassing that validation block entirely. |
|
@dgozman The block you link to is only for |
Problem
Running
npx playwright test --workers=0silently skips all tests and exits with code 0. This creates dangerous false-positives in CI pipelines — the build appears green even though nothing ran.Root cause:
resolveWorkers()returns the number as-is without validation, and the dispatcher loopfor (let i = 0; i < workers; i++)never executes.Fix
Added validation in
resolveWorkers()to throw a descriptive error for workers < 1:This applies to both numeric values and parsed string values. The percentage path was already safe (Math.max(1, ...) clamps to 1).
Testing
Fixes #39938