test: make single-task job fixtures assert their own output - #174
test: make single-task job fixtures assert their own output#174leongdl wants to merge 3 commits into
Conversation
Signed-off-by: David Leong <leongdl@amazon.com>
Signed-off-by: David Leong <leongdl@amazon.com>
e26ba4d to
bc609e1
Compare
| - name: OpenJDConformanceAssert | ||
| type: TEXT | ||
| data: | | ||
| # Self-asserting conformance wrapper. Runs `sys.argv[1:]` -- this case's original |
There was a problem hiding this comment.
Almost the whole diff appears to repeat this boilerplate. This likely reduces the readability of these tests for someone casually browsing these files. Are there other ways we could approach this?
There was a problem hiding this comment.
Yeah, thanks to claude Fable :) I have a new steering prompt to DELETE all this cruff.
Sorry I have not had time to review this one but it is necessary to make the tests robust and useful for backend.
There was a problem hiding this comment.
Done — the boilerplate is gone. The machinery now lives in one flat file, conformance-tests/_conformance_assert.py, and a fixture pulls it in with dataFile:. This case is now:
embeddedFiles:
- name: OpenJDConformanceAssert
type: TEXT
dataFile: _conformance_assert.py
- name: OpenJDConformanceExpect
type: TEXT
data: |
{"expected": ["OUTPUT:hello"]}Fixture insertions drop from 7,902 to 2,048, and a wrapper fix is now one edit instead of 187.
One thing I tried first and could not make work, since it would have needed no harness change at all: putting the expected lines directly in args and keeping the wrapper fully generic. 2023-09 format strings have no escape for {{, so the 15 cases that forbid a literal {{Param. in their output cannot express it there. Checked against both CLIs rather than assumed — a bare {{Param. is a template validation error, and neither \{\{ nor a doubled {{{{ escapes it; both pass through as those literal characters. JSON's own \uXXXX decodes the braces after the format-string pass, which is why the expect file is an embedded file rather than argv text.
The wrapper was copied verbatim into all 187 instrumented fixtures: one
distinct machinery body, 5,236 duplicated lines, 66% of what the change
added. Reviewers reasonably read that as boilerplate obscuring the tests.
The machinery now lives in conformance-tests/_conformance_assert.py and a
fixture pulls it in with `dataFile:` on its OpenJDConformanceAssert
embedded file; a harness resolves that to `data` before submitting. Each
case's own expected and forbidden lines move to a second embedded file,
OpenJDConformanceExpect, as one line of JSON.
Fixture insertions drop from 7,902 to 2,048, and a wrapper fix is now one
edit rather than 187.
The literals cannot instead ride in `args`, which would need no harness
change at all: 2023-09 format strings have no escape for `{{`, so the 15
cases that forbid a literal `{{Param.` cannot express it there. Confirmed
against both CLIs -- a bare `{{Param.` is a validation error, and neither
`\{\{` nor `{{{{` escapes it. JSON's own \uXXXX decodes the braces after
the format-string pass, so the expect file stays an embedded file.
`data` is required on an embedded file, so a harness that does not resolve
`dataFile` submits an invalid template and fails loudly instead of
silently reverting to a status-only verdict.
Verified against the pre-migration commit, per fixture and per platform:
the literals the task reads are identical to the ones the old wrapper
baked in (187/187), and the rest of every case -- original command, args,
`expected` block -- is unchanged (187/187). Those checks were themselves
mutation-tested; each of three deliberate corruptions was caught.
1160 passed, 0 failed on openjd-rs and openjd-cli 0.7.6, matching the
pre-migration baseline. Assertion confirmed to have executed in 180 of
187 on both, the other 7 being Windows-only and skipped on a POSIX host,
with zero cases passing without the success marker. The mutation probe --
corrupt what the job prints, leave the expected literal alone -- is
caught by the in-task assertion on both CLIs.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Update: the wrapper is now one shared flat filePushed The machinery was copied verbatim into all 187 instrumented fixtures: one distinct body, 5,236 duplicated lines, 66% of what the change added. It now lives in A fixture is now 8 lines of scaffolding: embeddedFiles:
- name: OpenJDConformanceAssert
type: TEXT
dataFile: _conformance_assert.py
- name: OpenJDConformanceExpect
type: TEXT
data: |
{"expected": ["OUTPUT:hello"]}
Why the literals are not simply in
|
carrier in args |
openjd-rs | openjd-cli 0.7.6 |
|---|---|---|
bare {{Param. |
rejected, validation error | rejected, validation error |
{{{{Param. |
rejected | rejected |
\{\{Param. |
passes through as those literal characters | same |
\u007b\u007bParam. |
passes through as those literal characters | same |
JSON's own \uXXXX escape decodes the braces after the format-string pass has run, so the expect file has to be an embedded file. That is the whole reason for the second file rather than two more argv elements.
Verification
Checked against the pre-migration commit, per fixture and per platform:
| result | |
|---|---|
| literals the task reads are identical to the ones the old wrapper baked in | 187 / 187 |
rest of the case unchanged — original command, args, expected block |
187 / 187 |
| expect file is valid JSON | 187 / 187 |
| fixtures that lost instrumentation | 0 |
Those checks are themselves mutation-tested, because a migration verifier that cannot fail is worse than none. Three deliberate corruptions — altered literal, altered original command, dropped expected_windows key — were each caught, and the tree was restored and re-verified clean afterwards.
Conformance, on the committed tree:
| openjd-rs | openjd-cli 0.7.6 | |
|---|---|---|
2023-09/* |
1160 passed, 0 failed | 1160 passed, 0 failed |
| assertion confirmed to have executed | 180 of 187 | 180 of 187 |
| cases passing without the success marker | 0 | 0 |
| mutation probe: corrupt what the job prints, keep the expected literal | caught in-task | caught in-task |
Both runs produce the same 14 per-suite counts, matching the pre-migration baseline. The 7 not confirmed are the Windows-only cases, skipped on a POSIX host.
The 6 fixtures whose subject is embedded files or Task.File now carry two of them; all 6 pass on both CLIs, so that hazard is closed by test rather than by argument.
Still open
- Windows is unrun. The
expected_windowsselection path is exercised by the migration verifier, which evaluates the wrapper for both platforms, but nothing has executed on a Windows host. Same gap this PR already carried for its 7 Windows-only cases; genuinely unverified until CI runswindows-latest. - Service-side harnesses need the
dataFileresolution before they bump their pinned conformance SHA. For a status-only consumer this is the change that keeps the content verdict working. It fails closed, so a harness that misses it goes red rather than quietly weaker — but it does have to land first.
CI is red, and it is not this PRAttribution checked before saying so. The failure profile on this PR is byte-identical to the one on mainline HEAD, which contains none of this instrumentation:
Same counts, same names. This PR adds no failures. The Python workflow was already red on the previous head Cause. Note also that Two of the names are cases this PR does instrument ( Locally, against an openjd-rs build that includes #336, this branch is 1160 passed, 0 failed — and the same on openjd-cli 0.7.6 (Python). Both with the same 14 per-suite counts as the pre-instrumentation baseline. Worth fixing separately: pinning |
What changed
187 single-task job fixtures across
base,EXPR,FEATURE_BUNDLE_1andREDACTED_ENV_VARSnow assert their own output, andrun_openjd_cli_tests.pynowrequires a valid job test to exit
0.1160 passed, 0 failed on openjd-rs and on openjd-cli 0.7.6. The diff is purely
additive: 187 files, +7856, −0.
Why
expected.outputis only checkable by a runner that can read the implementation'sstdout. An implementation that can only observe task status — a service rather than
a CLI whose pipe the runner holds — gets "a task ran and exited 0", not "it produced the
right answer".
{{Param.Version}}resolving to2.0instead of1.0exits 0 andpasses.
Each instrumented case's
onRuntakes one leading argument, anOpenJDConformanceAssertembedded file, followed by its original command and args:The wrapper runs
sys.argv[1:], echoes the output verbatim, compares it againstliterals baked into itself, and exits non-zero on mismatch.
expected.output/expected.forbiddenare untouched, so a log-scanning runner is unaffected — bothmechanisms now check the same thing by different means.
In-idiom rather than new:
expected.taskFailurealready lets exit status carry averdict.
928 of 1554 declared assertion lines are now checked inside the task:
baseEXPRFEATURE_BUNDLE_1REDACTED_ENV_VARSWRAP_ACTIONSTASK_CHUNKINGWhy the argv is passed through rather than embedded
Embedding the original argv in the wrapper's source works for
baseand would havequietly gutted several EXPR cases.
expr1.3.2--list-flattens-in-argsassertsARG0:--width…COUNT:10; its subject is how the implementation expands an argslist, flattening a list value into separate arguments and skipping a null one. Freezing
argv into the wrapper moves that expansion from the implementation into the fixture, so
the case keeps passing while testing nothing.
Passing argv through leaves the expansion where it belongs, and removes every escaping
hazard embedding had.
Two false passes found on the way
The runner never checked exit status for a valid case. A mutation probe — corrupt
what the job prints, leave the expected literal alone — passed:
Two independent causes, both fixed. The runner ignored
returncodewhen noexpected.taskFailurewas declared, so the verdict rested entirely on substringmatching; and the wrapper's diagnostic echoed the expected literal, so the log scan
matched the error message. Diagnostics now report
expected output line 1 of 2 not foundand never the text. The probe reports✗on both CLIs now.The exit-status requirement is a hole in the shared runner independent of the fixtures,
so it is a separate commit.
46 wrappers that never ran. All 47 instrumentable
WRAP_ACTIONScases passed afterinstrumentation, then the success marker showed 46 of them had not executed the wrapper
at all:
onWrapTaskRunreplaces the task action, so a hook likeargs: ["-c", "print('TIMEOUT={{WrappedAction.Timeout}}')"]never runsWrappedAction.Command. Shipping those would have added verification that looks presentand cannot fail — the same failure one layer up.
WRAP_ACTIONSis excluded wholesale.That is what
OPENJD_CONFORMANCE_ASSERT_OK: <n> expected, <m> forbiddenis for, and whyit is worth keeping.
Deliberately left status-only
4--openjd-redacted-envexpectsSECRET_IS:********, the implementation's redaction of what the task printed. The task sees the real value.unset-takes-precedenceis the one that does self-assert, because there the variable really is unset.7.3--env-file-referenceasserts a line printed by a job environment'sonEnter; its task prints something else.WRAP_ACTIONSwrap-no-argsWrappedAction.Argssurfaces as an empty list when the action has no args. Adding an argument changes the subject. Encoded as a rule: an action with no args is never instrumented.expected.taskFailurecasesEvery one of these except the first was found by instrumenting the case and watching it
fail, or watching it pass without asserting — not by reading the fixture.
Testing
2023-09/*2023-09/*✗on both;✓before the fixesexpectedpreservedThe 7 not confirmed are Windows-only and skipped on a POSIX host
(
7.3--path-param-mapping-windows,7.3--rawparam-no-mapping-windows, fourexpr2.3.2--uri-*-windows,6.1--end-of-line-auto-windows). Each has a POSIX siblingthat passes and differs only in which literal list the wrapper selects, so the risk is
low — but they are genuinely unverified until CI runs them on
windows-latest.