Add end-to-end ProcessAsync test for transient-pause (required + non-required)#436
Merged
Conversation
…required) The P1b transient-pause fix (#434) was covered only compositionally: the unit tests mock the phase to throw the transient directly, and HalibutResumeReattachTests calls the strategy directly — neither runs the real ExecuteSingleActionAsync per-action catch THROUGH the runner to the pause outcome. The non-required path (no per-target catch) was the gap the adversarial review flagged. This drives the REAL pipeline: IDeploymentTaskExecutor.ProcessAsync over a seeded deployment (KubernetesAgent target, so the real HalibutMachineExecutionStrategy records the in-flight pointer), with only the Halibut RPC transport faked (GetStatus throws a transient HalibutClientException). It asserts, for BOTH required and non-required steps: task Paused (not Failed), checkpoint preserved, and the in-flight pointer preserved. Red-green verified: with the ExecuteSingleActionAsync transient catch neutered, the NON-required case fails (Paused → Failed + checkpoint deleted) — the exact swallow bug — while the required case still passes (the per-target catch covers it), confirming the test specifically guards the non-required propagation the fix added.
… contrast
Replace the brittle Contains("\"t\"") substring with the production typed
reader InFlightScriptMap.TryGet(json, DispatchSlot(...)) asserting the preserved
pointer holds the EXACT ScriptTicket the strategy dispatched (captured in the
fake) — pins slot identity + ticket value and survives a JsonPropertyName rename.
Add a permanent-failure contrast case: the same seeded path with a permanent
Halibut subtype (ServiceNotFoundHalibutClientException) must Fail terminally +
delete the checkpoint, so the Paused rows read as "Paused iff transient" and the
transient/permanent boundary is pinned end-to-end, not just in the predicate's
own unit test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HalibutResumeReattachTestscalls the strategy directly — neither runs the realExecuteSingleActionAsyncper-action catch through the runner to the pause outcome. The non-required path (which has no per-target catch) was the exact gap the adversarial review flagged.IntegrationTransientPausedrives the real pipeline: a fullIDeploymentTaskExecutor.ProcessAsyncover a seeded deployment (KubernetesAgent target → the realHalibutMachineExecutionStrategyrecords the in-flight pointer), with only the Halibut RPC transport faked (GetStatusthrows a transientHalibutClientException). For both required and non-required steps it asserts: task Paused (not Failed), checkpoint preserved, in-flight pointer preserved.Test plan
[InlineData(true)](required) and[InlineData(false)](non-required) green against the real pipelineExecuteSingleActionAsynctransient catch makes the NON-required case fail (Paused→Failed, checkpoint deleted — the swallow bug) while the required case still passes (covered by the per-target catch) — proving the test specifically guards the non-required propagation the fix addedExecute.csunchanged