review feedback

packages/playwright/src/common/config.ts

@@ -100,16 +100,7 @@ export class FullConfigInternal {
     (this.config as any)[configInternalSymbol] = this;
 
     const workers = takeFirst(configCLIOverrides.workers, userConfig.workers, '50%');
-    if (typeof workers === 'string') {
+    this.config.workers = typeof workers === 'string' ? parseWorkers(workers) : workers;
-      if (workers.endsWith('%')) {
-        const cpus = os.cpus().length;
-        this.config.workers = Math.max(1, Math.floor(cpus * (parseInt(workers, 10) / 100)));
-      } else {
-        this.config.workers = parseWorkers(workers);
-      }
-    } else {
-      this.config.workers = workers;
-    }
 
     const webServers = takeFirst(userConfig.webServer, null);
     if (Array.isArray(webServers)) { // multiple web server mode
@@ -222,10 +213,16 @@ function resolveReporters(reporters: Config['reporter'], rootDir: string): Repor
 }
 
 function parseWorkers(workers: string) {
+  if (workers[workers.length - 1] === '%') {
+    if (/[^0-9]/.test(workers.slice(0, -1)))
+      throw new Error(`Workers ${workers} must be a number or percentage.`);
+    const percentage = parseInt(workers.slice(0, -1), 10);
+    const cpus = os.cpus().length;
+    return Math.max(1, Math.floor(cpus * (percentage / 100)));
+  }
   const parsedWorkers = parseInt(workers, 10);
   if (isNaN(parsedWorkers))
     throw new Error(`Workers ${workers} must be a number or percentage.`);
-
   return parsedWorkers;
 }
 

packages/playwright/src/program.ts

@@ -333,25 +333,30 @@ function resolveReporter(id: string) {
 }
 
 class OptionValidators {
-  static validateWorkers(value: string): string {
+  static validateNumber(value: string, options: { min?: number, max?: number }): string {
-    if (value.endsWith('%'))
+    if (/[^0-9]/.test(value))
-      return value;
-    if (isNaN(parseInt(value, 10)))
       throw new InvalidArgumentError('Not a number.');
+    const parsed = parseInt(value, 10);
+    if (isNaN(parsed))
+      throw new InvalidArgumentError('Not a number.');
+    if (options.min !== undefined && parsed < options.min)
+      throw new InvalidArgumentError(`Expected a number greater than ${options.min}.`);
+    if (options.max !== undefined && parsed > options.max)
+      throw new InvalidArgumentError(`Expected a number less than ${options.max}.`);
     return value;
   }
 
-  static validateNumber(value: string): string {
+  static validateWorkers(value: string): string {
-    if (isNaN(parseInt(value, 10)))
+    if (value[value.length - 1] === '%') {
-      throw new InvalidArgumentError('Not a number.');
+      OptionValidators.validateNumber(value, { min: 1, max: 100 });
+      return value;
+    }
+    OptionValidators.validateNumber(value, { min: 1 });
     return value;
   }
 
   static validateShard(value: string): string {
-    if (!value.includes('/'))
+    if (!/^\d+\/\d+$/.test(value))
-      throw new InvalidArgumentError('Shard should be in the form "current/all"');
-    const [current, all] = value.split('/').map(v => parseInt(v, 10));
-    if (isNaN(current) || isNaN(all))
       throw new InvalidArgumentError('Shard should be in the form "current/all"');
     return value;
   }
@@ -373,21 +378,21 @@ const testOptions: [string, string, ((value: string) => string | number)?][] = [
   ['--ignore-snapshots', `Ignore screenshot and snapshot expectations`],
   ['--last-failed', `Only re-run the failures`],
   ['--list', `Collect all the tests and report them, but do not run`],
-  ['--max-failures <N>', `Stop after the first N failures`, OptionValidators.validateNumber],
+  ['--max-failures <N>', `Stop after the first N failures`, value => OptionValidators.validateNumber(value, { min: 0 })],
   ['--no-deps', 'Do not run project dependencies'],
   ['--output <dir>', `Folder for output artifacts (default: "test-results")`],
   ['--pass-with-no-tests', `Makes test run succeed even if no tests were found`],
   ['--project <project-name...>', `Only run tests from the specified list of projects, supports '*' wildcard (default: run all projects)`],
   ['--quiet', `Suppress stdio`],
-  ['--repeat-each <N>', `Run each test N times (default: 1)`, OptionValidators.validateNumber],
+  ['--repeat-each <N>', `Run each test N times (default: 1)`, value => OptionValidators.validateNumber(value, { min: 0 })],
   ['--reporter <reporter>', `Reporter to use, comma-separated, can be ${builtInReporters.map(name => `"${name}"`).join(', ')} (default: "${defaultReporter}")`],
-  ['--retries <retries>', `Maximum retry count for flaky tests, zero for no retries (default: no retries)`, OptionValidators.validateNumber],
+  ['--retries <retries>', `Maximum retry count for flaky tests, zero for no retries (default: no retries)`, value => OptionValidators.validateNumber(value, { min: 0 })],
   ['--shard <shard>', `Shard tests and execute only the selected shard, specify in the form "current/all", 1-based, for example "3/5"`, OptionValidators.validateShard],
-  ['--timeout <timeout>', `Specify test timeout threshold in milliseconds, zero for unlimited (default: ${defaultTimeout})`, OptionValidators.validateNumber],
+  ['--timeout <timeout>', `Specify test timeout threshold in milliseconds, zero for unlimited (default: ${defaultTimeout})`, value => OptionValidators.validateNumber(value, { min: 0 })],
   ['--trace <mode>', `Force tracing mode, can be ${kTraceModes.map(mode => `"${mode}"`).join(', ')}`],
   ['--ui', `Run tests in interactive UI mode`],
   ['--ui-host <host>', 'Host to serve UI on; specifying this option opens UI in a browser tab'],
-  ['--ui-port <port>', 'Port to serve UI on, 0 for any free port; specifying this option opens UI in a browser tab', OptionValidators.validateNumber],
+  ['--ui-port <port>', 'Port to serve UI on, 0 for any free port; specifying this option opens UI in a browser tab', value => OptionValidators.validateNumber(value, { min: 0, max: 65_535 })],
   ['-u, --update-snapshots', `Update snapshots with actual results (default: only create missing snapshots)`],
   ['-j, --workers <workers>', `Number of concurrent workers or percentage of logical CPU cores, use 1 to run in a single worker (default: 50%)`, OptionValidators.validateWorkers],
   ['-x', `Stop after the first failure`],