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

fix: 100% coverage in tests #4607

Merged
merged 2 commits into from
Mar 24, 2022
Merged

fix: 100% coverage in tests #4607

merged 2 commits into from
Mar 24, 2022

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Mar 23, 2022

  • Removed dead code in lib/utils/usage.js.
  • Removed dead code in lib/base-command.js.
  • Removed "load-all" test, we currently have 100% coverage and new PRs
    without tests will be rejected if they don't add coverage for new
    files.
  • Removed check-coverage script as a separate command.
  • Removed separate coverage test in ci.yml.
  • Removed coverage flag from tap config, the default is already to
    enforce 100% coverage.

Added another commit that cleans up usage, which removing the dead code from base-command revealed as being needed.

  • Removed usage lib, rolled logic into base-command.js
  • Cleaned up usage output to be less redundant

@wraithgar wraithgar requested a review from a team as a code owner March 23, 2022 18:22
@npm-robot
Copy link
Contributor

npm-robot commented Mar 23, 2022

found 1 benchmarks with statistically significant performance regressions

  • app-large: no-clean:audit
timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 54.614 ±2.72 36.328 ±0.73 22.541 ±0.57 25.401 ±1.45 3.528 ±0.05 3.611 ±0.00 2.925 ±0.09 14.539 ±0.01 2.855 ±0.03 3.980 ±0.02
#4607 56.995 ±0.58 36.619 ±0.05 22.444 ±0.53 26.387 ±0.83 3.681 ±0.02 3.749 ±0.11 2.924 ±0.02 15.505 ±0.31 2.969 ±0.06 4.521 ±0.31
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 39.577 ±0.37 27.423 ±0.52 15.887 ±0.58 17.303 ±0.39 3.297 ±0.05 3.353 ±0.05 2.993 ±0.07 11.237 ±0.27 2.744 ±0.02 3.776 ±0.02
#4607 41.724 ±0.71 28.645 ±0.07 16.589 ±0.01 18.132 ±0.15 3.346 ±0.04 3.403 ±0.07 2.987 ±0.04 11.062 ±0.17 2.743 ±0.01 3.821 ±0.03

@ruyadorno ruyadorno added semver:patch semver patch level for changes Release 8.x work is associated with a specific npm 8 release release: next These items should be addressed in the next release labels Mar 24, 2022
 * Removed dead code in `lib/utils/usage.js`.
 * Removed dead code in `lib/base-command.js`.
 * Removed "load-all" test, we currently have 100% coverage and new PRs
   without tests will be rejected if they don't add coverage for new
   files.
 * Removed `check-coverage` script as a separate command.
 * Removed separate coverage test in ci.yml.
 * Removed `coverage` flag from tap config, the default is already to
   enforce 100% coverage.

Removed a tiny bit of dead code resulting from this
Removed usage lib, rolled logic into base-command.js
Cleaned up usage output to be less redundant
@fritzy fritzy merged commit 716a07f into latest Mar 24, 2022
@fritzy fritzy deleted the gar/coverage-map-update branch March 24, 2022 20:23
@lukekarrys lukekarrys mentioned this pull request Mar 31, 2022
@lukekarrys lukekarrys mentioned this pull request Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants