Update tap to version 14.3.1 by richardlau · Pull Request #135 · nodejs/node-report · GitHub
Skip to content
This repository was archived by the owner on Jun 18, 2021. It is now read-only.

Update tap to version 14.3.1#135

Open
richardlau wants to merge 2 commits into
nodejs:mainfrom
richardlau:auditfix
Open

Update tap to version 14.3.1#135
richardlau wants to merge 2 commits into
nodejs:mainfrom
richardlau:auditfix

Conversation

@richardlau

@richardlau richardlau commented Jul 4, 2019

Copy link
Copy Markdown
Member

Fixes npm audit warning for handlebars:

-bash-4.2$ npm audit

                       === npm audit security report ===

# Run  npm install --save-dev tap@14.4.0  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).

@richardlau
Add hacks to allow tests to run with coverage enabled
96a1bed 
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

richardlau commented Jul 4, 2019

Copy link
Copy Markdown
Member Author

@richardlau

richardlau commented Jul 4, 2019

Copy link
Copy Markdown
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
Merged
@richardlau
Update tap to version 14.3.1
1693f81 
Fixes `npm audit` warning for `handlebars`.
Disable `esm` to allow tests to pass on Node.js 13 nightlies.
@richardlau

richardlau commented Jul 5, 2019

Copy link
Copy Markdown
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

richardlau commented Jul 5, 2019

Copy link
Copy Markdown
Member Author

mhdawson
mhdawson approved these changes Jul 8, 2019
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

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
@richardlau richardlau mentioned this pull request Jan 22, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@richardlau @mhdawson