Skip to content

Run the shaded jar from the Node.js wrapper, and give it a test suite that can fail - #18

Merged
marevol merged 2 commits into
mainfrom
fix/nodejs-jar-selection
Sep 5, 2026
Merged

Run the shaded jar from the Node.js wrapper, and give it a test suite that can fail#18
marevol merged 2 commits into
mainfrom
fix/nodejs-jar-selection

Conversation

@marevol

@marevol marevol commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

The defect

nodejs/index.js picks the jar to run by taking the first file in target/
whose name merely contains yuicompressor:

if (path.extname(item) === '.jar' && item.indexOf('yuicompressor') !== -1) {

maven-shade-plugin leaves the pre-shade jar right next to the shaded one:

 66,719 target/original-yuicompressor-2.4.11-SNAPSHOT.jar   <- no dependencies inside
1,493,729 target/yuicompressor-2.4.11-SNAPSHOT.jar          <- the runnable one

Running the first one dies immediately:

Exception in thread "main" java.lang.NoClassDefFoundError: org/kohsuke/args4j/CmdLineException

Which of the two wins depends on readdir order, so the package works or
returns nothing depending on the filesystem. On this machine the broken one
wins, and compressor.compress() returns err === null with '' as the
compressed output.

package.json also shipped both jars to npm, so the broken one travelled
with the published package.

Why it shipped: the Node.js tests could not fail

Four independent reasons, each sufficient on its own:

  1. tests/node/tests.js required '../nodejs/index'. From tests/node/ that
    resolves to tests/nodejs/index, which does not exist - 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 none, so the suite was empty.
  3. yuitest exits 0 when no tests load. Actual output today:
    [ERROR] Cannot find module '../nodejs/index' ... followed by exit code 0.
  4. The CI job that ran it carried continue-on-error: true.

Change

  • Accept only yuicompressor-<version>.jar; reject original-*, -sources
    and -javadoc.
  • Exclude target/original-*.jar from the npm package (npm pack: 7 files -> 6).
  • Replace the dead suite with tests/node/wrapper.test.js on the built-in
    node:test runner, covering what the wrapper actually owns: which jar it
    selects, and whether compression reaches the caller. The yuitest
    devDependency is dropped - no dependency is needed.
  • npm test runs 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:

not ok 1 - picks the shaded jar, not a side artifact
  error: 'selected the pre-shade jar: original-yuicompressor-2.4.11-SNAPSHOT.jar'
not ok 2 - compresses JavaScript      expected: 'function hello(a){var b="hi, ";return b+a}'  actual: ''
not ok 3 - compresses CSS             expected: 'a{color:#fff;margin:0}'                      actual: ''
not ok 4 - compresses a string        expected: 'var x=1;'                                    actual: ''
# pass 0  # fail 4

and pass after the change (# pass 4 # fail 0). npm test exits 1 with a
failing test and 0 otherwise - the property the old suite did not have. The Java
suite is unaffected (819 tests, 0 failures).

… 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.
@marevol

marevol commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The nodejs-test (20.x) job failed on the first run, which is this PR working as
intended: node --test only accepts glob patterns from node 21 onward, so on
node 20 the pattern was taken as a literal path.

> node --test "tests/node/**/*.test.js"
Could not find '/home/runner/work/yuicompressor/yuicompressor/tests/node/**/*.test.js'
##[error]Process completed with exit code 1.

engines.node is >=20.0.0 and the CI matrix covers 20.x, so the script has to
work there. Fixed by letting the shell expand tests/node/*.test.js, which
hands node concrete paths on every version.

Worth noting because it is the point of the change: under the previous
continue-on-error: true this would have been a green build.

Verified on node 20.19.0 and 22.14.0 - 4 tests pass on both, and npm test
exits 1 with a failing test and 0 without one on both.

@marevol marevol added this to the 2.4.11 milestone Sep 5, 2026
@marevol marevol self-assigned this Sep 5, 2026
@marevol
marevol merged commit 5170b15 into main Sep 5, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant