fix: validate --object format in test command - #1097
Conversation
| ]; | ||
| if (opts.object) { | ||
| if (!opts.object.includes('.')) { | ||
| throw new Error(`Invalid --object format: expected object.method (or pkg.object.method), got "${opts.object}"`); |
There was a problem hiding this comment.
The guard works, but the issue asks for "at least two dot-separated segments". includes('.') is weaker: it still lets malformed values like app. or .works-fine through, producing a broken -Dtest=...# (empty method/object segment) rather than rejecting them. Suggest the stricter check that matches the issue wording:
| throw new Error(`Invalid --object format: expected object.method (or pkg.object.method), got "${opts.object}"`); | |
| if (opts.object.split('.').filter(Boolean).length < 2) { |
| if (opts.object) { | ||
| if (!opts.object.includes('.')) { | ||
| throw new Error(`Invalid --object format: expected object.method (or pkg.object.method), got "${opts.object}"`); | ||
| } |
There was a problem hiding this comment.
Per our TDD rule this needs a reproducing test for #1086 — a case with a single-segment --object (e.g. app) asserting that a validation Error is thrown. The feature file maps 1:1 to test/commands/java/test_test.js, which already covers the happy paths. At 3 hits-of-code the PR is also well under the 40 floor, so please add a few edge-case tests (single segment, trailing-dot app., leading-dot .works-fine) to both bulk it up and lock in the behavior. take a look please
Favixx
left a comment
There was a problem hiding this comment.
The filter(Boolean) answer to @Thayorns' point is the right idea, and it does close app. and .works-fine. But the guard and the parser five lines below it now disagree about what a segment is, and inputs exist that satisfy the guard and still produce the broken -Dtest that #1086 is about.
a..b → the guard filters to ['a','b'], length 2, passes. The parser then splits without filtering, parts.pop() twice gives method='b' and obj='', and the argument becomes -Dtest=org.eolang.EOa.EO*Test#b — a wildcard class name that matches every test in the package.
.a.b → same, passes the guard, and the empty leading segment becomes a package: -Dtest=org.eolang.EO.EOa*Test#b.
So the fix converts two crashes into two clear errors and leaves two silent misfires, which is the failure mode #1086 complains about. The three new tests cannot catch it because all three inputs (app, app., .works-fine) reduce to a single non-empty segment — the guard's filtered view and the parser's raw view only diverge when there are two real segments and an empty one.
The red CI is not this PR. executes a single Java unit test fails on macOS and Windows by running eoc test with no --object at all, so the block this PR adds never executes; master has been green all day, including two runs in the last two hours. Rebasing would clear it.
| `-Dheap-size=${opts.heap}`, | ||
| ]; | ||
| if (opts.object) { | ||
| if (opts.object.split('.').filter(Boolean).length < 2) { |
There was a problem hiding this comment.
This validates a filtered view of the input and line 29 then parses the unfiltered one, so the two disagree on exactly the inputs that contain an empty segment next to two real ones.
'a..b' → filter ['a','b'] length 2 → passes
→ parts ['a','','b'] → obj = '' → -Dtest=org.eolang.EOa.EO*Test#b
'.a.b' → filter ['a','b'] length 2 → passes
→ parts ['','a','b'] → pkg = 'EO' → -Dtest=org.eolang.EO.EOa*Test#b
Both run Maven with a selector that silently matches the wrong tests rather than failing — the same shape of problem as the TypeError in #1086, one layer further in.
Two ways to make the two views agree by construction. Either parse from the filtered array (const parts = opts.object.split('.').filter(Boolean); and drop the separate guard into a length check on it), or express the whole rule once as a pattern:
if (!/^[^.]+(\.[^.]+)+$/.test(opts.object)) { throw ... }The regex rejects leading, trailing and doubled dots and requires at least two segments, which is precisely what @Thayorns asked for, and it leaves nothing for the parser to disagree with.
| `Invalid --object format: expected object.method (or pkg.object.method), got "${opts.object}"` | ||
| ); | ||
| } | ||
| const parts = opts.object.split('.'); |
There was a problem hiding this comment.
This is the line the guard above is meant to protect, and it is the one that still sees the raw split.
Worth noting that even after the mismatch is fixed, parts.pop() twice followed by parts.map(...) is doing three jobs at once — taking the method, taking the object, and treating whatever is left as the package. Naming the pieces from a single destructure would make the shape obvious and leave no room for the two-view problem to come back:
const segments = opts.object.split('.').filter(Boolean);
const method = segments.pop();
const obj = segments.pop();
const pkg = segments;The - to _ replacement applies to all three the same way, so it can be one helper rather than three inline .replace(/-/g, '_') calls.
| } | ||
| const parts = opts.object.split('.'); | ||
| const method = parts.pop().replace(/-/g, '_'); | ||
| const obj = parts.pop().replace(/-/g, '_'); |
There was a problem hiding this comment.
obj is the value that becomes the class name, and it is the one that silently degrades: when it ends up empty, cls is EO*Test and the selector matches every test class in the package instead of failing.
A wildcard that broad is worth guarding even independently of the input validation — after the parse, obj and method are both required to be non-empty for the result to mean anything. One assertion there would catch any future parsing change, not just the current input shapes.
| const pkg = parts.map((p) => `EO${p.replace(/-/g, '_')}`).join('.'); | ||
| const cls = `EO${obj}*Test`; | ||
| args.push(`-Dtest=${pkg ? `org.eolang.${pkg}.${cls}` : `org.eolang.${cls}`}#${method}`); | ||
| args.push( |
There was a problem hiding this comment.
This reflow is unrelated to the fix — the argument is unchanged, only wrapped across three lines.
It is a small thing on its own, but combined with the reformatting of three untouched tests in the test file it makes the diff read as larger than the change is: 76 added lines of which roughly 40 are new tests and the rest is layout. On a repository that measures PR size, that is worth keeping separate.
| assert.strictEqual(captured.target, 'target'); | ||
| assert.strictEqual(captured.batch, true); | ||
| }); | ||
| it('throws on single-segment object', () => { |
There was a problem hiding this comment.
This is the reproducing test #1086 asked for and it does its job — against master this throws the TypeError the issue reports, and with the fix it throws the validation Error.
One structural caveat worth knowing: assert.throws works here only because the validation runs before elapsed(...) returns a promise. The other tests in this file await test(...), so the function is promise-returning; the moment the check moves inside the async callback these three tests will pass while asserting nothing, because the rejection would need assert.rejects. A comment saying the check is deliberately synchronous, or asserting on the rejected promise instead, removes that trap.
| heap: '256M', | ||
| sources: 'src', | ||
| target: 'target', | ||
| object: 'app', |
There was a problem hiding this comment.
All three new cases are inputs that reduce to one non-empty segment: app, app., .works-fine. That is one equivalence class, tested three times.
The class that is missing is two real segments plus an empty one — a..b and .a.b — which is exactly where the guard and the parser diverge (see test.js:24). Adding those two as assert.throws cases would fail today and pass once the guard and the parse agree, which makes them the regression tests for the fix rather than for the crash.
A fourth case worth having on the passing side: foo.bar.app.works-fine, to pin that a multi-segment package still joins correctly, since parts.map(...) is the only branch no test exercises with more than one package element.
| /Invalid --object format/ | ||
| ); | ||
| }); | ||
| it('throws on trailing dot', () => { |
There was a problem hiding this comment.
These three tests call test({...}) with a single argument, so maven falls back to the real mvnw default rather than the capturing stub the tests above pass.
Harmless today for the same reason assert.throws works — the throw happens before maven is ever touched. But it is the second thing in these tests that silently depends on the validation staying where it is, and unlike the promise issue it would not fail loudly: a regression would shell out to Maven from a unit test. Passing the same (args) => { captured = args; } stub the other tests use costs one line and removes the dependency.
| /Invalid --object format/ | ||
| ); | ||
| }); | ||
| it('throws on leading dot', () => { |
There was a problem hiding this comment.
.works-fine is a good case to have, and it is worth being explicit in the test name about why it throws, because the reason is not the leading dot as such.
It throws because after filtering there is one segment left. .a.b also has a leading dot and does not throw. So "throws on leading dot" describes the input rather than the rule, and the next person adding a case may reasonably conclude that leading dots are handled and skip .a.b. Something like throws when fewer than two segments remain names the actual condition and makes the gap visible.
Fixes #1086
Added validation to ensure --object value contains at least one dot
before splitting. If not, throws a clear error message instead of
crashing with an unhandled TypeError.