fix!: remove old audit fallback request#7911
Conversation
|
Test coverage failures are due to two things: Two functions which always get passed an object, and never get defaulted now, and uncovered lines in the packument cache because of missing headers in the mock registry. |
wraithgar
force-pushed
the
gar/audit-fallback
branch
6 times, most recently
from
0927b81 to
bc020ec
Compare
BREAKING CHANGE: npm will no longer fall back to the old audit endpoint if the bulk advisory request fails. This legacy code has a long tail in npm. Getting rid of it was difficult because of how load-bearing some of those requests were in tests. This PR removes the old "mock server" that arborist tests spun up, and moved that logic into the existing mock registry that the cli uses. This will allow us to consolidate our logic in tests, and also outline more granularly which tests actually make registry requests. A few tests that were testing just the fallback behavior were also removed.
wraithgar
force-pushed
the
gar/audit-fallback
branch
from
bc020ec to
8f91e46
Compare
|
|
||
| const printReified = (path, opt) => reify(path, opt).then(printTree) | ||
| const fixtures = join(__dirname, '..', 'fixtures') | ||
| const createRegistry = (t, mocks) => { |
There was a problem hiding this comment.
just a thought, no action required
This is convenient on one hand but tedious to remember to include for each test on the other. I'd suggest in the future using a beforeEach (and when needed, scoping registry to describe()) and group tests by describe(), that way it wouldn't need to be defined every test. This is especially relevant if the test itself doesn't use the returned value.
This would group the tests into four different types of describe() "buckets" :
createRegistry(t, true)createRegistry(t)const registry = createRegistry(t)const registry = createRegistry(t, true)
And you'd never need to use this within the file again. There is a caveat for one of the below examples that calls this with a loop, that would need to be handled differently.
There was a problem hiding this comment.
I envisioned these mocks moving into the mock-registry folder and then being hard set when you called registry.mocks. The changes in this PR were the smallest possible to get things moved over.
reggi
left a comment
reggi
left a comment
There was a problem hiding this comment.
This is a much needed, heavy lift and is a really elegant way of mocking and handling the audit endpoints. Thanks for this! 👍 🚀
3 participants
BREAKING CHANGE: npm will no longer fall back to the old audit endpoint
if the bulk advisory request fails.
This legacy code has a long tail in npm. Getting rid of it was
difficult because of how load-bearing some of those requests were in
tests. This PR removes the old "mock server" that arborist tests spun
up, and moved that logic into the existing mock registry that the cli
uses. This will allow us to consolidate our logic in tests, and also
outline more granularly which tests actually make registry requests.
A few tests that were testing just the fallback behavior were also
removed.
Closes: npm/statusboard#900