Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test, build: Clean up package-lock.json after make coverage #15196

Closed
wants to merge 2 commits into from

Conversation

ssbrewster
Copy link
Contributor

@ssbrewster ssbrewster commented Sep 5, 2017

When running make coverage, npm installs istanbul-merge and nyc and
generates a package-lock.json file.

This PR adds the rm package-lock.json command to the Makefile.

Refs: https://github.com/nodejs/node/pull/15190/files#r136932786

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Sep 5, 2017
@ssbrewster ssbrewster changed the title test: Clean up package-lock.json after make coverage test, build: Clean up package-lock.json after make coverage Sep 5, 2017
@lpinca
Copy link
Member

lpinca commented Sep 5, 2017

Isn't better to use the --no-package-lock flag?

@Fishrock123
Copy link
Contributor

Yeah I think --no-package-lock on the install would be more appropriate here overall.

@Fishrock123
Copy link
Contributor

Actually, relatedly, since npm now saves by default, make coverage should probably have --no-save(?) too.

When running make coverage npm installs istanbul-merge and nyc and
generates a package-lock.json file

Ensure this file is removed when running make coverage-clean

Refs: https://github.com/nodejs/node/pull/15190/files#r136932786
Use these flags when running make coverage so that a package-lock.json
file is not generated and npm does not attempt to save the deps to a
non-existent package.json file

Refs: https://github.com/nodejs/node/pull/15190/files#r136932786
@ssbrewster
Copy link
Contributor Author

Thanks for mentioning those flags @lpinca @Fishrock123. PR updated.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2017

@ssbrewster thanks a lot for your contribution!

Landed in 9168b8c

@BridgeAR BridgeAR closed this Sep 8, 2017
BridgeAR pushed a commit that referenced this pull request Sep 8, 2017
Use these flags when running make coverage so that a package-lock.json
file is not generated and npm does not attempt to save the deps to a
non-existent package.json file.

PR-URL: #15196
Refs: https://github.com/nodejs/node/pull/15190/files#r136932786
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@ssbrewster ssbrewster deleted the coverage-clean-pkg-lock branch September 8, 2017 06:13
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Use these flags when running make coverage so that a package-lock.json
file is not generated and npm does not attempt to save the deps to a
non-existent package.json file.

PR-URL: #15196
Refs: https://github.com/nodejs/node/pull/15190/files#r136932786
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
Use these flags when running make coverage so that a package-lock.json
file is not generated and npm does not attempt to save the deps to a
non-existent package.json file.

PR-URL: #15196
Refs: https://github.com/nodejs/node/pull/15190/files#r136932786
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Use these flags when running make coverage so that a package-lock.json
file is not generated and npm does not attempt to save the deps to a
non-existent package.json file.

PR-URL: #15196
Refs: https://github.com/nodejs/node/pull/15190/files#r136932786
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Use these flags when running make coverage so that a package-lock.json
file is not generated and npm does not attempt to save the deps to a
non-existent package.json file.

PR-URL: nodejs#15196
Refs: https://github.com/nodejs/node/pull/15190/files#r136932786
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@ssbrewster
Copy link
Contributor Author

ssbrewster commented Sep 21, 2017

@MylesBorins what is the criteria for backporting? Happy to raise a PR if needed.

@lpinca
Copy link
Member

lpinca commented Sep 21, 2017

@ssbrewster see https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md

@lpinca
Copy link
Member

lpinca commented Sep 21, 2017

I actually realized you asked another thing, sorry. I see no coverage target on v6.x Makefile so I think this should not be backported.

@ssbrewster
Copy link
Contributor Author

Ok thanks for the confirmation @lpinca 😄

@lpinca
Copy link
Member

lpinca commented Sep 21, 2017

Added dont-land label, sorry again for the confusion @ssbrewster.

@ssbrewster
Copy link
Contributor Author

No worries @lpinca

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants