Improves test scripts, cross-platform compatibility and async test patterns#629
Open
uguraslan wants to merge 2 commits intolightning-js:devfrom
Open
Improves test scripts, cross-platform compatibility and async test patterns#629uguraslan wants to merge 2 commits intolightning-js:devfrom
uguraslan wants to merge 2 commits intolightning-js:devfrom
Conversation
Test Results: ✅ PASSEDRun at: 2026-03-06T22:59:20.677Z Summary: |
Test Results: ✅ PASSEDRun at: 2026-03-07T07:58:52.630Z Summary: |
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.
While investigating some test related problems I found a few issues that needed fixing.
scripts/runTests.jstest:ciscripts/prepublishOnly.jsassert.end()in async test callbacksscripts/prepublishOnly.test.jsRemoval of
scripts/runTests.jsThis script was originally added to work around a bash extglob pattern that doesn't work on Windows. The problem is it spawns tape as a child process which causes different event loop timing. This was silently masking some test failures depending on how tests were run.
Replaced it with a direct tape CLI call using explicit glob patterns that tape expands internally:
No custom runner needed, no buffering, no child process overhead.
Tested on Windows 11 Pro and MacOS.
Updated test scripts, removed
test:citest:cihadc8on top oftest:runbut the GitHub workflows were using their own hardcoded commands withc8anyway, sotest:ciwas never actually called from CI. It wasn't adding any value, just sitting there as an unused script.Updated CI workflows
With
runTests.jsthe workflow was parsing its custom output format (passed: N failed: N) to detect failures. Switched to:steps.run-tests.outcome) as the failure signal. With this change we can detect silently failing tests due any problem so it is more reliable# tests,# pass,# fail) for the PR comment summaryAlso updated
master-coverage.ymlto usenpm run test:runinstead of its own hardcoded tape command so it stays in sync.Fix
scripts/prepublishOnly.jsWhen the precompiler processes a non-Blits file it returns
undefined. Without a guard this meant ESLint was being called on every non-Blits file unconditionally. AddednewSource !== undefinedcheck before comparing with source.Fix
assert.end()in async test callbackstape docs are clear on this:
An
asyncfunction always returns a Promise soassert.end()should never be called inside one. Went through all 21 async callbacks across three files and removed them:src/application.mouse.test.js: 9 async tests, removedassert.end()from eachsrc/router/router.test.js: 10 async tests, removedassert.end(). 1 test had noawaitat all so converted it to a sync callbackscripts/prepublishOnly.test.js: 1 async test (Setup), removedassert.end()Improved tests in
scripts/prepublishOnly.test.jsWhile going through the async test cleanup, I noticed that the
processDirectorytests were missing theexecStub. The stub forchildProcess.execwas only set up in theSetuptest but never passed down or recreated in the individualprocessDirectorytests. So realexeccalls were running. AddedexecStub = sinon.stub(childProcess, 'exec')andexecStub.restore()to each of those tests.Also rewrote the
formatFileWithESLinttests. They were just checking that the function doesn't throw. Replaced them with proper behavior tests that verify the exact ESLint command being called, the error handling (stderr output, error messages), and the silent success case.