Closed raphinesse closed 3 years ago
Merging #91 (dace3fc) into master (1d96eae) will increase coverage by
0.90%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
+ Coverage 92.42% 93.33% +0.90%
==========================================
Files 1 1
Lines 66 60 -6
==========================================
- Hits 61 56 -5
+ Misses 5 4 -1
Impacted Files | Coverage Δ | |
---|---|---|
index.js | 93.33% <100.00%> (+0.90%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 1d96eae...dace3fc. Read the comment docs.
Motivation and Context
https://github.com/apache/cordova-lib/issues/859
Description
Instead of parsing the installed package's name out of the output of
npm install <spec>
(which does not work anymore withnpm@7
) we now do the following:npm-package-arg
. This is very cheap.pacote
.Both of these libraries are also used by npm itself.
Fetching the manifest can be expensive (e.g. for a git repo spec). But since
pacote
andnpm
share a cache and we always install a package after fetching a manifest anyway, the additional cost should be negligible in our case. Measuring the execution time of this repo's tests on my machine seems to confirm this: the time is roughly the same before and after this PR.Furthermore this PR makes some adjustments to test expectations to handle minor behavior changes in
npm@7
.Testing
npm@6
andnpm@^7.2
npm@^7.2
(The one that doesn't, is due to https://github.com/npm/cli/issues/2339 and unrelated to the changes here)TODO
npm@7
uninstall
seems to be broken with--no-save
onnpm@7
(https://github.com/npm/cli/issues/2309)Do something about the test that is failing due to https://github.com/npm/cli/issues/2309Using fixed npm 7.2 nowCheck ifMy latest tests show very similar running times. That's good enough for me.pacote
andnpm
are sharing a cache as expected. The test duration increased from 90s to 110–120s with this change. So we are actually spending a bit more extra time than expected.Maybe replace a few of the unit tests I threw out here:-1: :man_shrugging: