Uh oh!
There was an error while loading. Please reload this page.
test: make run-log scenarios API-safe and stop mock drift - #357
Conversation
The two run-log scenarios asserted on strings only the mock API server ever emitted, so they passed locally and failed in the monorepo's CLI regression job, which runs this suite against a real Tower server. The post-completion drain scenario now deploys a fixture app that prints the two lines itself, and the follow scenario asserts a warning the server actually sends. The mock's log stream is aligned with the server: it closes with the idle warning and a terminal end-of-stream event, and its stored log lines match what the hello-world fixture prints.
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
konstantinoscs
left a comment
There was a problem hiding this comment.
My human review looks good.
Sol marked these. I leave them up to you:
[P2] Terminal sentinel is emitted while the run is still active. generate_normal_log_streamsends end_of_stream=true after 100 ms, while ordinary mock runs remain running for five seconds. The CLI reconnects and replays the warnings; a diagnostic assertion saw 9 copies of each warning. Gate the sentinel on terminal status and ideally assert it is consumed exactly once.
Two additional robustness concerns:
- logs_after_completion.py prints its second line before process exit. A real-server run can therefore pass without exercising the post-completion drain.
- The warning assertion uses an instantaneous app. If it finishes before --follow attaches, the CLI takes the stored-log path and emits no SSE warning. A deliberately long-running fixture would remove that race.
What this changes
Two scenarios in
cli_runs.featureasserted on log content that only the mock API server ever produced.First log before run completesandWarning: This run is using a deprecated runtimeexist nowhere in the Tower server, so the scenarios could only ever pass against the mock.That matters because the monorepo runs this exact suite against a real Tower server in its CLI regression job. It clones the tower-cli tag matching the installed PyPI version, so when v0.3.71 shipped these assertions, the monorepo's
developwent red and stays red on every push. See tower-monorepo run 32238254228.This PR makes both scenarios assert on things a real run produces:
Warning: No new logs available, which is one of only three warningssendRunLogNotificationscan send. It is guaranteed ahead of any stream close, because the terminal-status check sits inside the same branch that emits it.I also pulled the mock back in line with the server, since the drift is what let this happen in the first place. Its log stream now closes the way the real one does, with the idle warning followed by a terminal end-of-stream event, and its stored log lines match what the hello-world fixture actually prints.
Starting application...andApplication completed successfullywere mock fiction that nothing asserted on.History
This is the second time around for these assertions. They arrived in #120 in October, and #198 in February removed them for exactly this reason, under the title "align BDD tests and mock server with real API behavior". #353 put them back in August. Keeping the mock honest is the part that stops a third round.
Verification
I ran the suite against a real local Tower server, via
test-cli.shwithCLI_BRANCHpointed at this branch, which is the same path the monorepo's CLI regression job takes. Against the same server, develop fails 3 of 8 scenarios and this branch fails 1::35logs that arrive after run completes:49follow without duplicates:58finished run, stored logs exactly onceBoth scenarios this PR touches now pass against the real API, including the post-completion drain. The develop run also confirms the warning choice, since the server emitted
Warning: No new logs availableand thenWarning: stream complete, which is what the mock now does too.:58fails the same way on both branches, so it is pre-existing and not from this change. It passes in CI and fails locally, because it waits a fixed 2 seconds before reading stored logs and that is not long enough on a cold runner building a uv environment. Worth fixing separately, and I left it alone here to keep this PR to the drift problem.The full feature also passes against the mock, so both sides of the suite are green.