Skip to content

test(cli): pin grandchild output capture and exit propagation - #320

Open
leongdl wants to merge 1 commit into
OpenJobDescription:mainfrom
leongdl:conformance-single-task-self-assert
Open

test(cli): pin grandchild output capture and exit propagation#320
leongdl wants to merge 1 commit into
OpenJobDescription:mainfrom
leongdl:conformance-single-task-self-assert

Conversation

@leongdl

@leongdl leongdl commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

What changed

Two openjd-cli integration tests, plus a self_asserting_task.yaml fixture, pinning
that an onRun action which spawns its own child process has that grandchild's output
captured, and that the action's non-zero exit fails the run.

Why

openjd-specifications#174
makes 39 single-task conformance fixtures assert their own output: the action runs the
case's command as a child, echoes its output, and exits non-zero on mismatch. The
suite's verdict for those cases now depends on two behaviours of ours that nothing here
pinned:

behaviour test
A grandchild's output reaches our captured output test_grandchild_output_captured_and_assertion_passes
A non-zero action exit fails the run test_assertion_failure_fails_the_run

Regressing either would not fail this repo's suite — it would quietly weaken 39
conformance cases into "the task ran", which is exactly the false-pass shape that PR
set out to remove. The fixture takes a job parameter so the second test drives a real
mismatch rather than asserting on a contrived failure.

No behavioural change; both tests pass against main as-is.

Testing

result
cargo test --workspace --exclude openjd-for-js all pass, 0 failed
New tests 2 passed
cargo fmt --all -- --check clean
cargo clippy --release --all-features --all-targets --workspace --exclude openjd-for-js -- -D warnings clean
Conformance 2023-09/* with specifications#174 1160 passed, 0 failed

Worth noting for anyone re-running conformance locally: target/release/openjd has to
be rebuilt after syncing main. A stale binary reported
3.4--wide-int-range-expression as failing, which reads exactly like a real divergence
and is not one — that fix is already in main.

Signed-off-by: David Leong <leongdl@amazon.com>
@leongdl
leongdl requested a review from a team as a code owner August 15, 2026 17:03

/// The conformance suite's single-task job fixtures assert their own output: the
/// `onRun` action spawns the case's command as a child, reproduces its output, and
/// exits non-zero when that output does not match. That only holds if we capture a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The grandchild-output property this module says it pins is not actually exercised.

In self_asserting_task.yaml the action script runs subprocess.run(COMMAND, capture_output=True, ...) and then re-writes completed.stdout / completed.stderr to its own stdout/stderr. The grandchild's pipes are read and closed by the intermediate Python process — openjd only ever sees the direct child's fds. So this test would still pass if openjd dropped grandchild output entirely; it only re-verifies direct-child capture, which is already covered elsewhere in this file.

To actually pin the stated invariant, the grandchild has to inherit the action's stdout/stderr rather than have them captured, e.g. drop capture_output=True and check the returncode instead:

completed = subprocess.run(COMMAND)  # grandchild inherits our stdout/stderr

and then assert on openjd's captured stdout from the Rust side. As written, the second test (test_assertion_failure_fails_the_run) also largely duplicates the existing test_task_fails_still_exits_env exit-status coverage.

// Printed by the grandchild and echoed by the action. Missing means output
// from a process we did not spawn ourselves was dropped.
assert!(stdout.contains("OUTPUT:EXPECTED_VALUE"), "stdout: {stdout}");
assert!(!stdout.contains("ASSERT_FAILED"), "stdout: {stdout}");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion can never fire. The fixture writes ASSERT_FAILED to stderr (sys.stderr.write("ASSERT_FAILED: ...")), and the failure path is only reachable when the script exits 1 — which the preceding assert_eq!(code, 0) already rules out. The companion test correctly searches format!("{stdout}{stderr}"); this one should do the same (or just drop the check as redundant).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant