test: repr_py control-character conformance tests - #162
Conversation
There was a problem hiding this comment.
Quorum verdict: GOOD-WITH-NITS (4 reviewers flagged variants of the same two issues) — spec: EL §2.2.6 repr_py (newline must be escaped — spec gives the exact worked example) via RFC 0008 WrappedAction.Args forwarding.
(1) Spec self-tension, correctly disclosed but under-weighted: the fixture's own onRun args contain a literal U+000A, which §5.2's ArgString Cc-exclusion arguably forbids — as written the template may be spec-invalid, so promotion is contingent on resolving §5.2 vs the multi-line python -c convention (which the Deadline Docker container env also relies on; if §5.2 is enforced, that breaks service-wide). Resolve the spec question first.
(2) Attribution: README says "implementation bug (openjd-rs)" but the 2026-08-12 sweep shows it fails BOTH implementations — state Python's status.
(3) Partial overlap with PR #165's expr2.2.6--repr-py-newline-roundtrip (same root defect); the end-to-end wrap path justifies keeping both, but cross-reference them.
|
Quorum review (5 independent agents). 1 fixture + README. Verdict: GOOD-WITH-NITS. The repr_py newline defect is real (fails BOTH implementations per the 2026-08-12 sweep) and the end-to-end wrap-forwarding angle adds value over PR #165's unit-level twin — but the fixture is hostage to an unresolved spec tension its own README flags, and the README under-attributes the failure. Cross-PR: this branch uses component-level WRAP_ACTIONS/proposed/ while PR #166 uses kind-level job_templates/proposed/ — the five expected-failures PRs should standardize on one placement (kind-level routes promotion mechanically). |
8ad36ab to
cf644fb
Compare
|
Quorum-review fixes applied and pushed (rebased onto mainline 3069673):
|
openjd-rs#374 landed the repr_py escaping fix, so expr2.2.6--repr-py-newline-roundtrip passes and no longer belongs in proposed/. It is promoted to EXPR/jobs/ on OpenJobDescription#162 alongside its WRAP_ACTIONS twin, where the two land together and the control run against a pre-fix binary is recorded. Removing it here so the two copies do not collide when both merge. Its README section goes with it. The other two fixtures on this branch are untouched and still parked: both are spec decisions, not implementation bugs. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Spec-correct fixtures that FAIL against current reference implementations, parked in a proposed/ directory the conformance runner does not discover, so merging keeps the suite green. Each entry in proposed/README.md records the observed output, spec citation, and classification. Promote each fixture up one directory unchanged when its implementation fix lands. Companion to conformance-wrap-actions-gaps. Review: quorum-review fixes — kind-level proposed/ placement (jobs/proposed/); README corrected: the defect fails BOTH implementations (2026-08-12 sweep), the §5.2 ArgString conflict is now the stated promotion gate rather than a footnote (the fixture's own args are arguably spec-invalid until that is resolved), and the unit-level repr_py twin in the func-lib expected-failures PR is cross-referenced. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Both were parked as expected failures. openjd-rs#374 landed the fix, so they pass and become the regression tests the proposed/README promised. Measured against a binary built from openjd-rs main at 5b04959, and against a pre-fix binary as the control: fixture pre-fix merged EXPR/jobs/expr2.2.6--repr-py-newline-roundtrip FAIL PASS WRAP_ACTIONS/.../wrap-repr-py-escapes-newline FAIL PASS Both pre-fix failures are "SyntaxError: unterminated string literal" with the expected output missing, which is the defect they were written for: repr_py quoted its output without escaping, so a value carrying a newline emitted it raw and CPython would not parse the result. Discovered by the runner's directory scan from their new locations, not just single-file mode, which is what leaving proposed/ was for. No regression in either category: WRAP_ACTIONS 73 passed 0 failed, EXPR 352 passed 0 failed. The EXPR fixture comes from the func-lib expected-failures branch (OpenJobDescription#165), which should drop its copy so the two do not collide on merge. Headers rewritten, since both described the parked state and claimed the implementations fail. Now spec, test, expectation in three paragraphs each, naming the paired fixture, within the comment-line budget. proposed/README.md and the proposed/ directory are removed: the fixture it described was its only entry. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
8b8851f to
7189e46
Compare
windows-latest failed the Python lane on this fixture and ubuntu-latest passed the same installed CLI. Not an implementation difference: the fixture matched its own forbidden marker. The harness scans a job's whole output for forbidden substrings, and on Windows openjd-sessions-for-python echoes the full child command line at INFO (_subprocess.py logs list2cmdline(self._args)); the POSIX path logs only the temp .sh path. This fixture's script source contained the literal FAIL, and FAIL is its forbidden entry, so the echo matched even though every assertion passed. The run log discriminates the two cases. It reports "Found forbidden output: FAIL" with no "Missing expected output" line, so NEWLINE:PASS was printed and the escaping worked on Windows too. A genuine failure of this fixture looks different: the pre-fix control run reported the missing NEWLINE:PASS line alongside a SyntaxError. Split the marker across two adjacent literals, the convention this repo already uses for the same hazard (20c1a84, and the timeout fixture's 'SHOULD_NOT' + '_PRINT'). Python concatenates them at compile time, so the run-time value is unchanged at 'FAIL got=' while the source no longer carries the literal. Verified both halves: the value still compares equal, and the script body no longer contains FAIL. Verified the marker still catches a real failure: comparing against a wrong value makes the runner report 0 passed, 1 failed, and restoring the fixture byte-identically returns it to 1 passed, 0 failed. The Rust lane was never affected, because it prints only COMMAND_OUTPUT records while the command echo is tagged FILE_PATH|PROCESS_CONTROL. The WRAP_ACTIONS fixture is unaffected too: it declares no forbidden entries. Windows itself is unverified from here. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Two fixtures pinning Expression Language §2.2.6's requirement that
repr_pyescape control characters, promoted out ofproposed/now that the fix has landed.TLDR. Both were parked here as expected failures. openjd-rs#374 merged as
5b04959, so both pass and become the regression tests theproposed/README.mdpromised. Verified against a pre-fix binary as the control: both fail before the fix and pass after, so they pin what they claim.The two fixtures
EXPR/jobs/expr2.2.6--repr-py-newline-roundtriprepr_py("a\nb")fed toast.literal_evalround-trips to the inputWRAP_ACTIONS/jobs/wrap-repr-py-escapes-newline-in-wrapped-argsrepr_py(WrappedAction.Args)forwarding pattern, with a two-linepython -cbodyThe pair is deliberate. The EXPR one isolates the rule; the WRAP_ACTIONS one exercises the end-to-end path a template author actually writes, where the value crosses two process boundaries before being parsed.
Measured
Built
openjdfrom openjd-rsmainat5b04959, and separately from a pre-fix commit as the control:expr2.2.6--repr-py-newline-roundtripwrap-repr-py-escapes-newline-in-wrapped-argsBoth pre-fix failures are
SyntaxError: unterminated string literalwith the expected output missing, which is exactly the defect:repr_pyquoted its output without escaping, so a value carrying a newline emitted it raw and CPython would not parse the result.Run through the runner's directory scan from the new locations, not single-file mode, since discoverability is the point of leaving
proposed/:No regression in either category: WRAP_ACTIONS 73 passed, 0 failed and EXPR 352 passed, 0 failed.
Notes for review
The headers are rewritten rather than moved unchanged. Both described the parked state and asserted that the implementations fail, which is no longer true. They now carry spec, test and expectation in three paragraphs, name the paired fixture, and sit inside the comment-line budget.
proposed/README.mdand theproposed/directory are removed. The fixture it described was its only entry.The EXPR fixture originates on the func-lib expected-failures branch behind #165, which should drop its copy so the two do not collide on merge.
One thing this does not resolve, recorded so it is not lost: Template Schemas §5.2 restricts
<ArgString>to characters outside theCccategory, which excludes the newlines both of these fixtures rely on. Neither implementation enforces that, and 150 of 1,191 merged fixtures carry a newline in anargsentry, so the multi-linepython -cconvention is load-bearing suite-wide. Either §5.2 is relaxed to permitU+000AandU+000D, or that convention is invalid. That is a spec decision and a separate PR; these fixtures are correct under the reading both implementations already follow.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.