Skip to content

Fix subscription node tests for terminal error result contract - #10284

Merged
michaelstaib merged 3 commits into
mainfrom
mst/fix-subscription-terminal-error-tests
Aug 24, 2026
Merged

michaelstaib merged 3 commits into
mainfrom
mst/fix-subscription-terminal-error-tests

Conversation

@michaelstaib

Copy link
Copy Markdown
Member

No description provided.

Copilot AI lite review requested due to automatic review settings August 23, 2026 09:51

Copilot AI left a comment

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.

Pull request overview

Updates Fusion subscription execution node tests to align with the “terminal error result” contract, ensuring failures surface as a final OperationResult (with errors) followed by stream completion, and that memory arenas are disposed/retained correctly across failure scenarios.

Changes:

  • Adjusted subscription stream expectations to assert: one terminal error result then end of stream.
  • Added inline JSON snapshots and exception assertions for terminal error results.
  • Updated arena ownership/disposal assertions for “fails after minting” vs “fails before minting” scenarios.
Suppressed comments (2)

src/HotChocolate/Fusion/test/Fusion.Execution.Tests/Execution/OperationExecutionNodeTests.cs:115

  • enumerator.Current is captured for both firstResult and secondResult without first checking hasFirstResult/hasSecondResult. If either MoveNextAsync() returns false, Current is undefined and the test may throw before the assertions (and the finally will still attempt to dispose results). Assert the MoveNextAsync() outcome before reading Current, and remove the redundant asserts from the assert section.
        var hasFirstResult = await enumerator.MoveNextAsync();
        var firstResult = enumerator.Current;
        var mintedArena = client.MintedArenas.Single();
        var hasSecondResult = await enumerator.MoveNextAsync();
        var secondResult = enumerator.Current;
        var hasThirdResult = await enumerator.MoveNextAsync();

src/HotChocolate/Fusion/test/Fusion.Execution.Tests/Execution/OperationExecutionNodeTests.cs:124

  • This test now has 8 Assert.* calls, which exceeds the repo testing guideline hard limit of 5 asserts per test method (AGENTS.md:51). Consider consolidating the multiple checks (stream progression, error JSON, exception details, and arena state) into a snapshot (Markdown snapshot if multiple shapes) to stay within the limit.
            // the delivered event arrives, then one terminal error result, then the stream ends
            var firstArena = Assert.IsType<MemoryArena>(mintedArena);
            Assert.True(hasFirstResult);
            Assert.True(hasSecondResult);
            Assert.False(hasThirdResult);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@michaelstaib
michaelstaib merged commit 187981c into main Aug 24, 2026
3 of 4 checks passed
@michaelstaib
michaelstaib deleted the mst/fix-subscription-terminal-error-tests branch August 24, 2026 07:06
This was referenced Aug 29, 2026
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.

2 participants