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: remove test coverage map #4862

Merged
merged 1 commit into from
May 7, 2022
Merged

fix: remove test coverage map #4862

merged 1 commit into from
May 7, 2022

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented May 5, 2022

Turns out there were three files that still had no test coverage because
of the combination of the mocks in tests and the coverage map. Removing
the map altogether exposed them.

This PR removes the coverage map and fixes test to cover all lines that
were being missed.

While adding coverage to the npm search codebase multiple unneeded
guards and at least one bug was found (it was impossible to exclude
searches based on username). These were fixed.

The npm view tests were also refactored to use the real npm object.

@wraithgar wraithgar requested a review from a team as a code owner May 5, 2022 14:18
@wraithgar wraithgar force-pushed the gar/no-coverage-map branch from 2b9424a to 9be58d5 Compare May 5, 2022 14:26
@npm-robot
Copy link
Contributor

npm-robot commented May 5, 2022

no statistically significant performance changes detected

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 69.138 ±12.81 34.720 ±0.03 19.634 ±0.22 22.471 ±1.06 3.118 ±0.06 3.125 ±0.02 2.495 ±0.04 12.810 ±0.03 2.399 ±0.02 3.557 ±0.10
#4862 57.388 ±6.38 34.856 ±0.01 23.270 ±4.77 22.735 ±0.45 3.162 ±0.02 3.118 ±0.01 2.490 ±0.01 12.883 ±0.04 2.490 ±0.03 3.613 ±0.01
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 40.457 ±1.90 26.701 ±0.00 14.346 ±0.08 15.198 ±0.12 2.885 ±0.06 2.845 ±0.01 2.531 ±0.02 9.124 ±0.06 2.323 ±0.00 3.330 ±0.15
#4862 41.998 ±0.26 26.635 ±0.22 14.447 ±0.15 15.424 ±0.28 2.904 ±0.03 2.888 ±0.02 2.578 ±0.01 9.344 ±0.02 2.354 ±0.03 3.269 ±0.04

@wraithgar wraithgar force-pushed the gar/no-coverage-map branch 3 times, most recently from 3b70063 to f11fd1b Compare May 5, 2022 20:16
return []
}

module.exports = coverageMap
Copy link
Contributor

Choose a reason for hiding this comment

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

sweet

Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Turns out there were three files that still had no test coverage because
of the combination of the mocks in tests and the coverage map.  Removing
the map altogether exposed them.

This PR removes the coverage map and fixes test to cover all lines that
were being missed.

While adding coverage to the `npm search` codebase multiple unneeded
guards and at least one bug was found (it was impossible to exclude
searches based on username). These were fixed.

The `npm view` tests were also refactored to use the real npm object.

Finally, a small inlining of lib/utils/file-exists.js was done.
@wraithgar wraithgar force-pushed the gar/no-coverage-map branch from f11fd1b to eae9637 Compare May 5, 2022 20:42
@wraithgar wraithgar changed the title fix: remote test coverage map fix: remove test coverage map May 7, 2022
@wraithgar wraithgar merged commit 48d2db6 into latest May 7, 2022
@wraithgar wraithgar deleted the gar/no-coverage-map branch May 7, 2022 16:11
maricarlagumbay1 added a commit to maricarlagumbay1/cli that referenced this pull request May 8, 2022
@wraithgar wraithgar mentioned this pull request May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants