test(cli): pin grandchild output capture and exit propagation - #320
test(cli): pin grandchild output capture and exit propagation#320leongdl wants to merge 1 commit into
Conversation
Signed-off-by: David Leong <leongdl@amazon.com>
|
|
||
| /// 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 |
There was a problem hiding this comment.
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/stderrand 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}"); |
There was a problem hiding this comment.
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).
What changed
Two
openjd-cliintegration tests, plus aself_asserting_task.yamlfixture, pinningthat an
onRunaction which spawns its own child process has that grandchild's outputcaptured, 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:
test_grandchild_output_captured_and_assertion_passestest_assertion_failure_fails_the_runRegressing 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
mainas-is.Testing
cargo test --workspace --exclude openjd-for-jscargo fmt --all -- --checkcargo clippy --release --all-features --all-targets --workspace --exclude openjd-for-js -- -D warnings2023-09/*with specifications#174Worth noting for anyone re-running conformance locally:
target/release/openjdhas tobe rebuilt after syncing
main. A stale binary reported3.4--wide-int-range-expressionas failing, which reads exactly like a real divergenceand is not one — that fix is already in
main.