chore: misc testing fixes (#2930)

* chore: misc test fixes

* Sort test runs by os first

* Use cross-env for test env var

* Try sorting matrix params

* Make FAST_TEST the default and rename to FULL_TEST

* Separate helper functions to not need to export test obj in files

.github/PULL_REQUEST_TEMPLATE.md

@@ -7,7 +7,7 @@ Contributor guide: https://github.com/nodejs/node/blob/main/CONTRIBUTING.md
 ##### Checklist
 <!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->
 
-- [ ] `npm install && npm test` passes
+- [ ] `npm install && npm run lint && npm test` passes
 - [ ] tests are included <!-- Bug fixes and new features should include tests -->
 - [ ] documentation is changed or added
 - [ ] commit message follows [commit guidelines](https://github.com/googleapis/release-please#how-should-i-write-my-commits)

.github/workflows/tests.yml

@@ -18,11 +18,24 @@ jobs:
     - uses: actions/checkout@v4
     - run: pip install --user ruff
     - run: ruff --output-format=github --select="E,F,PLC,PLE,UP,W,YTT" --ignore="E721,PLC1901,S101,UP031" --target-version=py38 .
+  Lint_JS:
+    runs-on: ubuntu-latest
+    steps:
+    - name: Checkout Repository
+      uses: actions/checkout@v4
+    - name: Use Node.js 20.x
+      uses: actions/setup-node@v3
+      with:
+        node-version: 20.x
+    - name: Install Dependencies
+      run: npm install --no-progress
+    - name: Lint
+      run: npm run lint
   Engines:
     runs-on: ubuntu-latest
     steps:
     - name: Checkout Repository
-      uses: actions/checkout@v3
+      uses: actions/checkout@v4
     - name: Use Node.js 20.x
       uses: actions/setup-node@v3
       with:
@@ -41,10 +54,11 @@ jobs:
       fail-fast: false
       max-parallel: 15
       matrix:
-        node: [16.x, 18.x, 20.x]
+        os: [macos, ubuntu, windows]
         python: ["3.8", "3.10", "3.12"]
-        os: [macos-latest, ubuntu-latest, windows-latest]
-    runs-on: ${{ matrix.os }}
+        node: [16.x, 18.x, 20.x]
+    name: ${{ matrix.os }} - ${{ matrix.python }} - ${{ matrix.node }}
+    runs-on: ${{ matrix.os }}-latest
     steps:
       - name: Checkout Repository
         uses: actions/checkout@v4
@@ -63,7 +77,7 @@ jobs:
           npm install --no-progress
           pip install pytest
       - name: Set Windows environment
-        if: startsWith(matrix.os, 'windows')
+        if: matrix.os == 'windows'
         run: |
           echo 'GYP_MSVS_VERSION=2015' >> $Env:GITHUB_ENV
           echo 'GYP_MSVS_OVERRIDE_PATH=C:\\Dummy' >> $Env:GITHUB_ENV
@@ -77,7 +91,11 @@ jobs:
         if: runner.os != 'Windows'
         shell: bash
         run: npm test --python="${pythonLocation}/python"
+        env:
+          FULL_TEST: ${{ (matrix.node == '20.x' && matrix.python == '3.12') && '1' || '0' }}
       - name: Run tests (Windows)
         if: runner.os == 'Windows'
         shell: pwsh
         run: npm run test --python="${env:pythonLocation}\\python.exe"
+        env:
+          FULL_TEST: ${{ (matrix.node == '20.x' && matrix.python == '3.12') && '1' || '0' }}

bin/node-gyp.js

@@ -32,9 +32,9 @@ if (prog.devDir) {
 
 if (prog.todo.length === 0) {
   if (~process.argv.indexOf('-v') || ~process.argv.indexOf('--version')) {
-    console.log('v%s', prog.version)
+    log.stdout('v%s', prog.version)
   } else {
-    console.log('%s', prog.usage())
+    log.stdout('%s', prog.usage())
   }
   process.exit(0)
 }
@@ -82,12 +82,12 @@ async function run () {
 
     if (command.name === 'list') {
       if (args.length) {
-        args.forEach((version) => console.log(version))
+        args.forEach((version) => log.stdout(version))
       } else {
-        console.log('No node development files installed. Use `node-gyp install` to install a version.')
+        log.stdout('No node development files installed. Use `node-gyp install` to install a version.')
       }
     } else if (args.length >= 1) {
-      console.log(...args.slice(1))
+      log.stdout(...args.slice(1))
     }
 
     // now run the next command in the queue

lib/configure.js

@@ -1,14 +1,14 @@
 'use strict'
 
-const { openSync, closeSync, promises: fs } = require('graceful-fs')
+const { promises: fs } = require('graceful-fs')
 const path = require('path')
 const log = require('./log')
 const os = require('os')
 const processRelease = require('./process-release')
 const win = process.platform === 'win32'
 const findNodeDirectory = require('./find-node-directory')
-const createConfigGypi = require('./create-config-gypi')
-const { format: msgFormat } = require('util')
+const { createConfigGypi } = require('./create-config-gypi')
+const { format: msgFormat, findAccessibleSync } = require('util')
 const { findPython } = require('./find-python')
 const { findVisualStudio } = win ? require('./find-visualstudio') : {}
 
@@ -277,32 +277,5 @@ async function configure (gyp, argv) {
   }
 }
 
-/**
- * Returns the first file or directory from an array of candidates that is
- * readable by the current user, or undefined if none of the candidates are
- * readable.
- */
-function findAccessibleSync (logprefix, dir, candidates) {
-  for (let next = 0; next < candidates.length; next++) {
-    const candidate = path.resolve(dir, candidates[next])
-    let fd
-    try {
-      fd = openSync(candidate, 'r')
-    } catch (e) {
-      // this candidate was not found or not readable, do nothing
-      log.silly(logprefix, 'Could not open %s: %s', candidate, e.message)
-      continue
-    }
-    closeSync(fd)
-    log.silly(logprefix, 'Found readable %s', candidate)
-    return candidate
-  }
-
-  return undefined
-}
-
 module.exports = configure
-module.exports.test = {
-  findAccessibleSync
-}
 module.exports.usage = 'Generates ' + (win ? 'MSVC project files' : 'a Makefile') + ' for the current module'

lib/create-config-gypi.js

@@ -143,8 +143,8 @@ async function createConfigGypi ({ gyp, buildDir, nodeDir, vsInfo, python }) {
   return configPath
 }
 
-module.exports = createConfigGypi
-module.exports.test = {
+module.exports = {
+  createConfigGypi,
   parseConfigGypi,
   getCurrentConfigGypi
 }

lib/download.js

@@ -0,0 +1,39 @@
+const fetch = require('make-fetch-happen')
+const { promises: fs } = require('graceful-fs')
+const log = require('./log')
+
+async function download (gyp, url) {
+  log.http('GET', url)
+
+  const requestOpts = {
+    headers: {
+      'User-Agent': `node-gyp v${gyp.version} (node ${process.version})`,
+      Connection: 'keep-alive'
+    },
+    proxy: gyp.opts.proxy,
+    noProxy: gyp.opts.noproxy
+  }
+
+  const cafile = gyp.opts.cafile
+  if (cafile) {
+    requestOpts.ca = await readCAFile(cafile)
+  }
+
+  const res = await fetch(url, requestOpts)
+  log.http(res.status, res.url)
+
+  return res
+}
+
+async function readCAFile (filename) {
+  // The CA file can contain multiple certificates so split on certificate
+  // boundaries.  [\S\s]*? is used to match everything including newlines.
+  const ca = await fs.readFile(filename, 'utf8')
+  const re = /(-----BEGIN CERTIFICATE-----[\S\s]*?-----END CERTIFICATE-----)/g
+  return ca.match(re)
+}
+
+module.exports = {
+  download,
+  readCAFile
+}

lib/install.js

@@ -9,12 +9,13 @@ const { Transform, promises: { pipeline } } = require('stream')
 const crypto = require('crypto')
 const log = require('./log')
 const semver = require('semver')
-const fetch = require('make-fetch-happen')
+const { download } = require('./download')
 const processRelease = require('./process-release')
+
 const win = process.platform === 'win32'
 
 async function install (gyp, argv) {
-  console.log()
+  log.stdout()
   const release = processRelease(argv, gyp, process.version, process.release)
   // Detecting target_arch based on logic from create-cnfig-gyp.js. Used on Windows only.
   const arch = win ? (gyp.opts.target_arch || gyp.opts.arch || process.arch || 'ia32') : ''
@@ -410,41 +411,5 @@ class ShaSum extends Transform {
   }
 }
 
-async function download (gyp, url) {
-  log.http('GET', url)
-
-  const requestOpts = {
-    headers: {
-      'User-Agent': `node-gyp v${gyp.version} (node ${process.version})`,
-      Connection: 'keep-alive'
-    },
-    proxy: gyp.opts.proxy,
-    noProxy: gyp.opts.noproxy
-  }
-
-  const cafile = gyp.opts.cafile
-  if (cafile) {
-    requestOpts.ca = await readCAFile(cafile)
-  }
-
-  const res = await fetch(url, requestOpts)
-  log.http(res.status, res.url)
-
-  return res
-}
-
-async function readCAFile (filename) {
-  // The CA file can contain multiple certificates so split on certificate
-  // boundaries.  [\S\s]*? is used to match everything including newlines.
-  const ca = await fs.readFile(filename, 'utf8')
-  const re = /(-----BEGIN CERTIFICATE-----[\S\s]*?-----END CERTIFICATE-----)/g
-  return ca.match(re)
-}
-
 module.exports = install
-module.exports.test = {
-  download,
-  install,
-  readCAFile
-}
 module.exports.usage = 'Install node development files for the specified node version.'

lib/log.js

@@ -73,11 +73,11 @@ class Logger {
     style: { fg: 'red', bg: 'black' }
   }]
 
-  constructor () {
+  constructor (stream) {
     process.on('log', (...args) => this.#onLog(...args))
     this.#levels = new Map(this.#levels.map((level, index) => [level.id, { ...level, index }]))
     this.level = 'info'
-    this.stream = process.stderr
+    this.stream = stream
     procLog.pause()
   }
 
@@ -158,8 +158,12 @@ class Logger {
   }
 }
 
+// used to suppress logs in tests
+const NULL_LOGGER = !!process.env.NODE_GYP_NULL_LOGGER
+
 module.exports = {
-  logger: new Logger(),
+  logger: new Logger(NULL_LOGGER ? null : process.stderr),
+  stdout: NULL_LOGGER ? () => {} : (...args) => console.log(...args),
   withPrefix,
   ...procLog
 }

lib/util.js

@@ -1,8 +1,9 @@
 'use strict'
 
-const log = require('./log')
 const cp = require('child_process')
 const path = require('path')
+const { openSync, closeSync } = require('graceful-fs')
+const log = require('./log')
 
 const execFile = async (...args) => new Promise((resolve) => {
   const child = cp.execFile(...args, (...a) => resolve(a))
@@ -48,8 +49,33 @@ async function regSearchKeys (keys, value, addOpts) {
   }
 }
 
+/**
+ * Returns the first file or directory from an array of candidates that is
+ * readable by the current user, or undefined if none of the candidates are
+ * readable.
+ */
+function findAccessibleSync (logprefix, dir, candidates) {
+  for (let next = 0; next < candidates.length; next++) {
+    const candidate = path.resolve(dir, candidates[next])
+    let fd
+    try {
+      fd = openSync(candidate, 'r')
+    } catch (e) {
+      // this candidate was not found or not readable, do nothing
+      log.silly(logprefix, 'Could not open %s: %s', candidate, e.message)
+      continue
+    }
+    closeSync(fd)
+    log.silly(logprefix, 'Found readable %s', candidate)
+    return candidate
+  }
+
+  return undefined
+}
+
 module.exports = {
   execFile,
   regGetValue,
-  regSearchKeys
+  regSearchKeys,
+  findAccessibleSync
 }

package.json

@@ -38,13 +38,14 @@
   },
   "devDependencies": {
     "bindings": "^1.5.0",
+    "cross-env": "^7.0.3",
     "mocha": "^10.2.0",
     "nan": "^2.14.2",
     "require-inject": "^1.4.4",
     "standard": "^17.0.0"
   },
   "scripts": {
     "lint": "standard \"*/*.js\" \"test/**/*.js\" \".github/**/*.js\"",
-    "test": "npm run lint && mocha --timeout 15000 --reporter=test/reporter.js test/test-download.js test/test-*"
+    "test": "cross-env NODE_GYP_NULL_LOGGER=true mocha --timeout 15000 test/test-download.js test/test-*"
   }
 }

test/common.js

@@ -1,6 +1,7 @@
 const envPaths = require('env-paths')
+const semver = require('semver')
 
-module.exports.devDir = () => envPaths('node-gyp', { suffix: '' }).cache
+module.exports.devDir = envPaths('node-gyp', { suffix: '' }).cache
 
 module.exports.poison = (object, property) => {
   function fail () {
@@ -15,3 +16,9 @@ module.exports.poison = (object, property) => {
   }
   Object.defineProperty(object, property, descriptor)
 }
+
+// Only run full test suite when instructed and on a non-prerelease version of node
+module.exports.FULL_TEST =
+  process.env.FULL_TEST === '1' &&
+  process.release.name === 'node' &&
+  semver.prerelease(process.version) === null