feat: validate concurrency option

PR-URL: nodejs/node#43976 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> (cherry picked from commit 60da0a1b364efdd84870269d23b39faa12fb46d8)
nodejs · Jul 27, 2022 · f875da2 · f875da2
1 parent 8c95f07
commit f875da2
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 14 deletions.
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -1,4 +1,4 @@
-// https://github.com/nodejs/node/blob/dab492f0444b0a6ae8a41dd1d9605e036c363655/lib/internal/test_runner/test.js
+// https://github.com/nodejs/node/blob/60da0a1b364efdd84870269d23b39faa12fb46d8/lib/internal/test_runner/test.js
 
 'use strict'
 
@@ -36,9 +36,9 @@ const {
 } = require('#internal/util')
 const { isPromise } = require('#internal/util/types')
 const {
-  isUint32,
   validateAbortSignal,
-  validateNumber
+  validateNumber,
+  validateUint32
 } = require('#internal/validators')
 const { setTimeout } = require('#timers/promises')
 const { TIMEOUT_MAX } = require('#internal/timers')
@@ -152,14 +152,22 @@ class Test extends AsyncResource {
       this.timeout = parent.timeout
     }
 
-    if (isUint32(concurrency) && concurrency !== 0) {
-      this.concurrency = concurrency
-    } else if (typeof concurrency === 'boolean') {
-      if (concurrency) {
-        this.concurrency = isTestRunner ? MathMax(cpus().length - 1, 1) : Infinity
-      } else {
-        this.concurrency = 1
-      }
+    switch (typeof concurrency) {
+      case 'number':
+        validateUint32(concurrency, 'options.concurrency', 1)
+        this.concurrency = concurrency
+        break
+
+      case 'boolean':
+        if (concurrency) {
+          this.concurrency = isTestRunner ? MathMax(cpus().length - 1, 1) : Infinity
+        } else {
+          this.concurrency = 1
+        }
+        break
+
+      default:
+        if (concurrency != null) throw new TypeError(`Expected options.concurrency to be a number or boolean, got ${concurrency}`)
     }
 
     if (timeout != null && timeout !== Infinity) {

diff --git a/lib/internal/validators.js b/lib/internal/validators.js
@@ -1,4 +1,4 @@
-// https://github.com/nodejs/node/blob/d83446b4c4694322e12d2b7d22592f2be674e580/lib/internal/validators.js
+// https://github.com/nodejs/node/blob/60da0a1b364efdd84870269d23b39faa12fb46d8/lib/internal/validators.js
 function isUint32 (value) {
   return value === (value >>> 0)
 }
@@ -23,8 +23,24 @@ const validateAbortSignal = (signal, name) => {
   }
 }
 
+const validateUint32 = (value, name, positive) => {
+  if (typeof value !== 'number') {
+    throw new TypeError(`Expected ${name} to be a number, got ${value}`)
+  }
+  if (!Number.isInteger(value)) {
+    throw new RangeError(`Expected ${name} to be an integer, got ${value}`)
+  }
+  const min = positive ? 1 : 0
+  // 2 ** 32 === 4294967296
+  const max = 4_294_967_295
+  if (value < min || value > max) {
+    throw new RangeError(`Expected ${name} to be ${`>= ${min} && <= ${max}`}, got ${value}`)
+  }
+}
+
 module.exports = {
   isUint32,
   validateAbortSignal,
-  validateNumber
+  validateNumber,
+  validateUint32
 }
diff --git a/test/parallel/test-runner-option-validation.js b/test/parallel/test-runner-option-validation.js
@@ -1,4 +1,4 @@
-// https://github.com/nodejs/node/blob/d83446b4c4694322e12d2b7d22592f2be674e580/test/parallel/test-runner-option-validation.js
+// https://github.com/nodejs/node/blob/60da0a1b364efdd84870269d23b39faa12fb46d8/test/parallel/test-runner-option-validation.js
 
 'use strict'
 require('../common')
@@ -15,4 +15,16 @@ const test = require('node:test');
 [null, undefined, Infinity, 0, 1, 1.1].forEach((timeout) => {
   // Valid values should not throw.
   test({ timeout })
+});
+
+// eslint-disable-next-line symbol-description
+[Symbol(), {}, [], () => {}, 1n, '1'].forEach((concurrency) => {
+  assert.throws(() => test({ concurrency }), { code: 'ERR_INVALID_ARG_TYPE' })
+});
+[-1, 0, 1.1, -Infinity, NaN, 2 ** 33, Number.MAX_SAFE_INTEGER].forEach((concurrency) => {
+  assert.throws(() => test({ concurrency }), { code: 'ERR_OUT_OF_RANGE' })
+});
+[null, undefined, 1, 2 ** 31, true, false].forEach((concurrency) => {
+  // Valid values should not throw.
+  test({ concurrency })
 })