Run the shaded jar from the Node.js wrapper, and give it a test suite that can fail - #18
Merged
Conversation
… that can fail
nodejs/index.js selected the first file in target/ whose name merely contained
"yuicompressor". maven-shade-plugin leaves the pre-shade jar next to the shaded
one as original-yuicompressor-<version>.jar: 66 KB, no dependencies inside, and
running it dies with
NoClassDefFoundError: org/kohsuke/args4j/CmdLineException
Which of the two jars won depended on readdir order, so the wrapper worked or
returned nothing depending on the filesystem. The selection now accepts only
yuicompressor-<version>.jar and rejects -sources/-javadoc.
package.json shipped both jars to npm, so the broken one travelled with the
published package; it is excluded now (npm pack: 7 files -> 6).
Why this shipped: the Node.js test suite ran zero assertions and reported
success, for four independent reasons.
1. tests/node/tests.js required '../nodejs/index', which from tests/node/
resolves to tests/nodejs/index - a path that does not exist, so the file
could not load at all.
2. Had it loaded, it scanned tests/ for <name>.<ext> + <name>.<ext>.min
fixture pairs. That directory contains no such pairs.
3. yuitest exits 0 when no tests load.
4. The CI job running it carried continue-on-error: true.
It is replaced with tests/node/wrapper.test.js on the built-in node:test runner,
covering what the wrapper actually owns: which jar it picks, and whether
compression reaches the caller. The yuitest devDependency is dropped, npm test
exits non-zero on failure, and CI no longer ignores the result.
Verified: all four tests fail on the unfixed wrapper - the jar-selection test
naming original-yuicompressor-2.4.11-SNAPSHOT.jar, the three compression tests
with an empty string as actual output - and pass after the change. npm test
exits 1 with a failing test and 0 otherwise. The Java suite is unaffected.
`node --test` only accepts glob patterns from node 21 onward; on node 20 the
pattern is taken as a literal path and the run ends with
Could not find '.../tests/node/**/*.test.js'
exit code 1. package.json declares engines.node >= 20 and CI covers 20.x, so the
script has to work there. Letting the shell expand `tests/node/*.test.js` hands
node concrete paths and behaves identically on both.
This is the newly un-silenced nodejs-test job doing its job: the same mistake
under the previous `continue-on-error: true` would have been reported as a
passing build.
Verified on node 20.19.0 and node 22.14.0: 4 tests pass on both, and `npm test`
exits 1 with a failing test and 0 without one on both.
Contributor
Author
|
The
Worth noting because it is the point of the change: under the previous Verified on node 20.19.0 and 22.14.0 - 4 tests pass on both, and |
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.
The defect
nodejs/index.jspicks the jar to run by taking the first file intarget/whose name merely contains
yuicompressor:maven-shade-pluginleaves the pre-shade jar right next to the shaded one:Running the first one dies immediately:
Which of the two wins depends on
readdirorder, so the package works orreturns nothing depending on the filesystem. On this machine the broken one
wins, and
compressor.compress()returnserr === nullwith''as thecompressed output.
package.jsonalso shipped both jars to npm, so the broken one travelledwith the published package.
Why it shipped: the Node.js tests could not fail
Four independent reasons, each sufficient on its own:
tests/node/tests.jsrequired'../nodejs/index'. Fromtests/node/thatresolves to
tests/nodejs/index, which does not exist - the file could notload at all.
tests/for<name>.<ext>+<name>.<ext>.minfixture pairs. That directory contains none, so the suite was empty.
yuitestexits 0 when no tests load. Actual output today:[ERROR] Cannot find module '../nodejs/index'... followed by exit code 0.continue-on-error: true.Change
yuicompressor-<version>.jar; rejectoriginal-*,-sourcesand
-javadoc.target/original-*.jarfrom the npm package (npm pack: 7 files -> 6).tests/node/wrapper.test.json the built-innode:testrunner, covering what the wrapper actually owns: which jar itselects, and whether compression reaches the caller. The
yuitestdevDependency is dropped - no dependency is needed.
npm testruns it, and CI no longer ignores the result.The compressor itself is untouched.
Verification
All four tests fail on the unfixed wrapper, for the right reasons:
and pass after the change (
# pass 4 # fail 0).npm testexits 1 with afailing test and 0 otherwise - the property the old suite did not have. The Java
suite is unaffected (819 tests, 0 failures).