test: fix pre-existing unit-test failures (green the suite) - #179
Draft
07souravkunda wants to merge 1 commit into
Draft
test: fix pre-existing unit-test failures (green the suite)#17907souravkunda wants to merge 1 commit into
07souravkunda wants to merge 1 commit into
Conversation
Repair test rot and a harness bug in test/local.js so the suite runs to completion and reports per-test results instead of aborting mid-run. Harness fix: - Assertions inside asynchronous callbacks (the tree-kill callback from Local.stop, and the binary-download callbacks) run outside Mocha's synchronous try/catch. A throw there escaped as an uncaught exception and aborted the whole Mocha process, hiding every test that had not run yet. Added a small `check(done, assertions)` helper that runs the assertions in a try/catch and routes any failure through `done`, so a failing assertion is reported as a normal test failure and the run continues. Test rot fixed: - LocalBinary.binaryPath gained a `bsHost` parameter (conf, bsHost, key, parentRetries, callback), but the Retries specs still called the old 4-arg form, so the callback landed in the wrong slot and was never invoked -> the before() hook hung until its 10-minute timeout. Passed the missing bsHost argument. - The Retries spec asserted download() was called with a retry count of 5; the default retry budget is now 9 (baseRetries). Updated the expectation. - The "Download Path" specs called getDownloadPath() with the old synchronous no-arg signature and asserted a hard-coded public URL. getDownloadPath is now async and prefixes a dynamically fetched source URL; the OS/arch -> filename mapping they were really validating now lives in getBinaryFilename(). Rewrote them to assert getBinaryFilename() directly. Left red on purpose (real product behaviour, not masked): - "should stop local" asserts isRunning() === false immediately after stop()'s callback fires. stop() now sends SIGTERM via tree-kill and calls back as soon as the signal is dispatched, without waiting for the process to exit or clearing its own pid/isProcessRunning state, so isRunning() stays true for a few seconds. This is a genuine product race and is deliberately left failing rather than weakened. No production code changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Running
npm testonmasteraborted the whole Mocha process partwaythrough (an uncaught exception thrown from inside an asynchronous callback),
so most of the suite never ran and its results were hidden. Several other
specs had also rotted against changes in
lib/(method signatures and thedownload flow) and were failing or hanging. This makes it hard to tell
whether a red run is caused by a change under test or by the pre-existing
baseline.
This PR greens the deterministic unit tests and makes the run complete
cleanly, so a failing test can be trusted to mean something. Only
test/local.jschanges; no production code is touched.What was failing
Establishing a clean baseline on
master(npm test, mocha overtest/):Local > should stop local: the test's assertionruns inside the tree-kill callback fired by
Local.stop, i.e. outsideMocha's synchronous try/catch, so the throw became an uncaught exception
that killed the Node process. Every test after it (
Start sync,LocalBinary > *) was silently skipped.LocalBinary > Download Path(5 specs):TypeError: Cannot read properties of undefined (reading 'proxyHost').LocalBinary > Retries: thebefore()hook hung until its 10-minutetimeout.
What I fixed (test rot / harness only)
check(done, assertions)helper. Assertions that execute inside asynccallbacks (the
stoptree-kill callback, the binary-download callbacks)now run in a try/catch and route failures through
done, so a failingassertion is a normal single-test failure and the run continues to the end.
Retrieshang — stale method signature.LocalBinary.binaryPathgaineda
bsHostparameter —(conf, bsHost, key, parentRetries, callback)— butthe specs still called the old 4-arg form, so the callback landed in the
wrong slot and was never invoked, hanging
before(). Passed the missingbsHostargument.Retriesstale value. A spec asserteddownload()was invoked with aretry count of
5; the default budget is now9(baseRetries). Updatedthe expectation.
Download Path— stale API. These specs calledgetDownloadPath()withthe old synchronous no-arg signature and asserted a hard-coded public URL.
getDownloadPathis now asynchronous and prefixes a dynamically fetchedsource URL; the OS/arch → filename mapping they were actually validating now
lives in
getBinaryFilename(). Rewrote them to assertgetBinaryFilename()directly (renamed the block to
Binary filename).What I deliberately left red (real product behaviour — NOT masked)
Local > should stop local. It assertsisRunning() === falseimmediately after
stop()'s callback fires.stop()sendsSIGTERMviatree-kill and calls back as soon as the signal is dispatched — it does not
wait for the process to exit, and it never clears its own
pid/isProcessRunningstate. SoisRunning()keeps returningtruefor a few seconds after
stop()"completes" (it flips tofalseonly oncethe OS process actually dies). This is a genuine race in
stop()'scompletion semantics, so the test is left failing rather than weakened,
sleep-padded, or deleted. It should be addressed in
lib/Local.js(e.g. clear the running state in
stop, and/or resolve only after exit).Not addressed here (credentialed integration tests)
LocalBinary > Download(3 specs) download a real ~36 MB binary over thenetwork from an authenticated endpoint. They require a valid
BROWSERSTACK_ACCESS_KEYand network access, and currently never call backwithout one (the download helper only logs on a source-URL fetch error and
never invokes its callback, so the test waits out its 10-minute timeout).
These are integration tests, not offline unit tests, so they are left
untouched here rather than force-mocked.
How to run
The start/stop and download tests need a valid
BROWSERSTACK_ACCESS_KEYinthe environment and network access. With a key present, the suite now runs to
completion: the unit tests are green,
should stop localis the oneremaining product-behaviour failure described above, and the
Downloadintegration specs need network + credentials as noted.