Show the test suite some 💋💋💋 #591
Open
milesnash wants to merge 30 commits intolightning-js:masterfrom
Open
Conversation
Prior to this change, the import tree branching from method.test.js would cause keyMap to be accessed before instantiation
Tests were failing in this file due to component accessing props of Log before it was initialised. Also, use of non-existent assert.equals
Test Results: ❌ FAILEDRun at: 2026-02-02T05:51:04.743Z Summary: Error Output: |
Test Results: ✅ PASSEDRun at: 2026-02-02T09:23:29.449Z Summary: { total: 1002, pass: 1001, skip: 1 } |
Test Results: ✅ PASSEDRun at: 2026-02-02T09:45:47.989Z Summary: { total: 1002, pass: 1001, skip: 1 } |
Author
|
@michielvandergeest I'd be grateful for any feedback you might have about this work 🙏 |
Test Results: ✅ PASSEDRun at: 2026-02-18T09:43:45.853Z Summary: |
Test Results: ✅ PASSEDRun at: 2026-02-19T06:05:59.870Z 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.
Ok, bear with me on this one, let me explain.
I wanted to PR a minor, one-line fix but, whilst doing that, I wanted to add a test for it. Sooo, in order to do that I started by running the tests; and that's where this whole thing got started.
Running
npm run teston the current master outputs a bunch of lint and type errors, says 162 tests passed, 0 failed and exits with code 1. Runningnpm run test:runalso outputs the errors, say 311 tests passed, 0 failed and exits with code 1.npm run test:cibasically runstest:run. I was a bit confused.I wanted to make this better first and started investigating why there were errors and an error exit code. Long story short, I found that difficult and also discovered that the test runner, tape, hasn't had much code activity in the past year and the tap reporter, tap-diff, is archived and hasn't had any activity on it in 10 years.
So I thought it might be a good idea to find another tool that met the following criteria:
I landed on tap, which seemed to fulfil these criteria nicely.
This is obviously tap-compatible but a few things needed to be tweaked, including:
assert.true|falseweren't functions in tap, so I refactored them to theassert.equalequivalentassert.notEqual->assert.notassert.deepEqual->assert.sameassert.equals->assert.equalAfter this I still found that a bunch of tests just would not pass. They were failing in ways that really made me question what the current test suite was actually running. This included:
src/component/base/methods.test.js: the dependency tree stemming from this file causedkeyMapto be accessed before it was initialised. To fix this, I had to movekeyMapto its own module and change how we access it.scripts/prepublishOnly.test.js: this was trying to assert that an "orig" file existed, but it didn't.src/component/setup/computed.test.js,src/focus.test.js&src/router/router.test.js: these were failing because they tested files that attempted to call methods ofLogwhen it hadn't been initialisedsrc/components/Sprite.test.js: this was failing because the tested component was callingsetonundefinedsrc/engines/L3/element.test.js: this was failing because it was instantiating classes before they were definedsrc/lib/codegenerator/generator.test.js: this was failing because the generated code was different since___wrappergot introducedAfter all that tests run with tap passed! 🎉
I then did a bit of console capturing and removal of spurious console logging in the test files to get noise out of the test output. The result is much, much better-looking test output imo 😍
Next, I updated the test commands in the
package.json. The ci tests and the scary-looking output parsing scripts in them should continue to work, as the output is still standard tap output for that command.Finally, I removed the now-unused dev dependencies from the repo 😅