Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Update tap to version 14.3.1 #135

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

richardlau
Copy link
Member

Fixes npm audit warning for handlebars:

-bash-4.2$ npm audit

                       === npm audit security report ===

# Run  npm install --save-dev [email protected]  to resolve 1 vulnerability
SEMVER WARNING: Recommended action is a potentially breaking change
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ handlebars                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ tap [dev]                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ tap > nyc > istanbul-reports > handlebars                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/755                             │
└───────────────┴──────────────────────────────────────────────────────────────┘


found 1 high severity vulnerability in 578 scanned packages
  1 vulnerability requires semver-major dependency updates.
-bash-4.2$

Disable esm to allow tests to pass on Node.js 13 nightlies. (nodejs/node#28294, standard-things/esm#821).

tap now enables coverage by default and the update has exposed that a handful of tests have issues when run with coverage enabled (the same failures can be seen if run with the non-updated tap with --coverage). The main issue is that coverage is wrapping spawned child processes so that:

  • The spawned process runs something else which runs the tests (this is hidden on the JavaScript side from the tests but not from node-report which reports the actual command line).
  • Signal handlers are being installed by the wrapping which prevent the spawned process being terminated by signals (since the signals are now being caught).

The first commit contains changes to work around these issues. The alternative would be to disable coverage (which isn't as bad as it sounds given how little non-test/demo JavaScript is contained in this project).

When run through `tap` with coverage enabled (`--coverage`) child
processes are wrapped and signal handlers installed which prevent
some of the tests from exiting.

Add workarounds to the affected tests so that the tests can be run
with coverage enabled.
@richardlau
Copy link
Member Author

Also tap version 13 (this PR bumps from 12 to 14) dropped support for Node.js < 8 (tapjs/tapjs#498). This is only a dev-dependency and shouldn't affect the node-report module itself so I think we can release this in a normal patch release (but you may not be able to test node-report on the earlier End-of-Life Node.js releases). Please speak up if you disagree.

@richardlau richardlau mentioned this pull request Jul 5, 2019
Fixes `npm audit` warning for `handlebars`.
Disable `esm` to allow tests to pass on Node.js 13 nightlies.
@richardlau
Copy link
Member Author

Alternative PR in #136 which keeps tap at version 12 which maintains compatibility with Node.js 6.

Keeping this PR open for the next semver major bump when we can properly raise the minimum Node.js version to 8 (or 10 depending on when we do the bump).

@richardlau
Copy link
Member Author

Node.js 6 CI demonstrating the break: https://ci.nodejs.org/job/nodereport-continuous-integration-latest/39/

example Node.js 6 failure:

14:59:00 + npm install
14:59:38 [email protected] /home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-report
14:59:38 └── [email protected] 
14:59:38 
14:59:38 npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^2.0.6 (node_modules/chokidar/node_modules/fsevents):
14:59:38 npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
14:59:38 npm ERR! Linux 5.0.16-300.fc30.x86_64
14:59:38 npm ERR! argv "/home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-v6.17.1-linux-x64/bin/node" "/home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-v6.17.1-linux-x64/bin/npm" "install"
14:59:38 npm ERR! node v6.17.1
14:59:38 npm ERR! npm  v3.10.10
14:59:38 npm ERR! path /home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-report/node_modules/.staging/@types/prop-types-4055500f
14:59:38 npm ERR! code ENOENT
14:59:38 npm ERR! errno -2
14:59:38 npm ERR! syscall rename
14:59:38 
14:59:38 npm ERR! enoent ENOENT: no such file or directory, rename '/home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-report/node_modules/.staging/@types/prop-types-4055500f' -> '/home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-report/node_modules/tap/node_modules/@types/prop-types'
14:59:38 npm ERR! enoent ENOENT: no such file or directory, rename '/home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-report/node_modules/.staging/@types/prop-types-4055500f' -> '/home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-report/node_modules/tap/node_modules/@types/prop-types'
14:59:38 npm ERR! enoent This is most likely not a problem with npm itself
14:59:38 npm ERR! enoent and is related to npm not being able to find a file.
14:59:38 npm ERR! enoent 
14:59:38 
14:59:38 npm ERR! Please include the following file with any support request:
14:59:38 npm ERR!     /home/iojs/build/workspace/nodereport-continuous-integration-latest/nodes/fedora-latest-x64/node-report/npm-debug.log
14:59:39 Build step 'Execute shell' marked build as failure

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@richardlau richardlau added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants