test: record queued work after concurrency rejection

[luojiyin1987]

What changed

Move the regression test to runWithConcurrency in tests/helpers.test.ts.
The test uses three tasks and a limit of two.
Task 1 rejects while task 2 waits.
After the returned promise rejects, the test releases task 2 and checks that task 3 starts.
This records current default behavior pending #1300.
The PR changes no production code.

Refs #1298.

Test

npm test -- --runInBand tests/helpers.test.ts passed: 51 tests.
npm test -- --runInBand passed: 2278 tests.
npm run build passed.

Data

The test uses numbers and a local promise.
No user data required redaction.

[sonukapoor]

First, #1298 is a good bug report: the reproduction is exact and the behaviour is real. Thanks for separating the observation from the change.

The test discriminates, and I checked it the hard way. Applying stop-on-first-rejection to runWithConcurrency fails exactly one test in the whole suite, and it is this one, 1 failed against 2312 passed. It is also stable, three runs in a row under the default config rather than only --runInBand.

That same result is my main comment, though. One line in src/utils/array.ts decides the outcome, so what you are pinning is entirely a property of runWithConcurrency, not of the scanner. To get at it this test drives scanPackages, createAdvisorySource, a real OsvAdvisorySource, HTTP response plumbing, the cache loader, a prototype spy, a temp directory and a three second cleanup timeout, and none of that scaffolding is what fails when the behaviour changes. tests/helpers.test.ts already has a runWithConcurrency block with four cases, including propagates errors from individual tasks, and that case uses a limit equal to the item count so nothing is ever left queued at the moment of failure. Your case is the one that block is missing. Expressed there it is roughly eight lines with no fetch, no spy and no cleanup, and it breaks only for the reason it exists.

Two smaller things while you are in it. options.batchSize: "1" is not read on this path, because scanPackages takes the batch size as a positional argument, which is where the real 1 comes from. And expect(requested).toEqual([1, 2, 3, 4, 5]) silently encodes the hardcoded concurrency limit of 5 at the OSV batch call site; that limit is live territory under #837, so if someone retunes it this fails with a message that gives no hint the 5 was load-bearing. Name it rather than asserting it bare.

Last thing, and this one is mine to sort out rather than yours. #1300 says it has to update this test before it can merge, so as written this locks in behaviour you are simultaneously proposing to remove. I would still rather have the test, because #1300 touches the scan loop and that is a design call I need to make separately, and if it goes a different way your test is the record of what the code actually does. So please title and comment it to say it records current behaviour pending #1300, rather than reading as the behaviour we want. Then whichever way #1300 goes, the edit is a couple of lines instead of a deleted file.

[luojiyin1987]

Updated PR #1299 to address the review feedback:

• Moved the regression test into the existing runWithConcurrency test block.
• Removed the scanner, HTTP, cache, and spy dependencies.
• Made the concurrency limit explicit.
• Clarified that the test documents current default behavior pending feat: stop queued OSV batches after failure #1300.

All CI checks are passing.

Thanks for the detailed review. Ready for re-review.

[sonukapoor]

Both asks addressed, and the result is a much better test than the one I reviewed: five lines in the block that already covers this helper, no fetch, no prototype spy, no temp directory, no cleanup timeout. The title naming #1300 is the part that matters, since whichever way that goes nobody will misread this as a specification. Applying the stop-on-rejection behaviour fails this test and nothing else. Approved.

[sonukapoor]

Merged - thank you @luojiyin1987!