Skip to content

[Mocha] Stabilize saga integration tests against timing flakes - #9947

Merged
glen-84 merged 8 commits into
mainfrom
gai/stabilize-saga-integration-tests
Oct 9, 2026
Merged

glen-84 merged 8 commits into
mainfrom
gai/stabilize-saga-integration-tests

Conversation

@glen-84

@glen-84 glen-84 commented Jun 18, 2026 •

Copy link
Copy Markdown
Member

Summary

  • The saga IntegrationTests slept for a fixed time (Task.Delay) between steps and before asserting on store state, which flaked under CI load: the Main Branch Coverage run had Saga_Should_TimeoutWithCustomResponse fail with Expected: 0, Actual: 1.
  • A saga dispatches a transition's messages before its state is committed, and a correlated message that arrives before that commit finds no instance and is discarded, so no fixed delay makes these tests reliable. Each test now waits until the saga state its next message depends on is persisted before sending that message, then polls for the expected state or for the instance to be deleted, bounded by the existing 10 s timeout.
  • Saga_Should_ExecuteThroughSteps waits for Started to be persisted before the first TriggerEvent, and for the saga to finish consuming that event before sending the second. The in-memory saga store exposes a transition's new state before it commits, so the end of the consume, which comes after the commit, is the reliable signal. The timeout tests wait until the saga is persisted before sending SagaTimedOutEvent.
  • Saga_Should_SupportRequestResponse uses a new GatedTriggerRequestHandler that holds the reply, parking the saga in AwaitingResponse until its persisted state is observed; releasing the reply then drives finalization and deletion.

Test plan

  • IntegrationTests: 10/10 passing on net8.0, net9.0, net10.0, and net11.0.
  • Full Mocha.Sagas.Tests suite: 612 passed, 4 skipped, 0 failed across the four target frameworks.
  • With a temporary local saga store whose commit takes 2 s, the four tests still pass. With one that holds back a transition's commit until the next transition has committed, Saga_Should_ExecuteThroughSteps passes.

Copilot AI review requested due to automatic review settings June 18, 2026 10:53

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

Stabilizes Mocha saga integration tests by replacing fixed Task.Delay waits with deterministic synchronization against persisted saga state, reducing CI timing flakes.

Changes:

  • Replaced fixed delays with bounded polling that waits for specific saga persistence/deletion conditions.
  • Ensured timeout tests only send SagaTimedOutEvent after the saga instance is persisted.
  • Added a gated request handler to hold replies so the AwaitingResponse persisted state can be observed deterministically.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Mocha/test/Mocha.Sagas.Tests/IntegrationTests.cs Outdated
Comment thread src/Mocha/test/Mocha.Sagas.Tests/IntegrationTests.cs Outdated
Comment thread src/Mocha/test/Mocha.Sagas.Tests/IntegrationTests.cs Outdated
@github-code-quality

Copy link
Copy Markdown

Code Coverage Overview

Languages: C#

C# / code-coverage/dotnet

The overall coverage in the gai/stabilize-saga-i... branch remains at 49%, unchanged from the main branch.

Show a code coverage summary of the most impacted files.
File main 443ea28 gai/stabilize-saga-i... b9841b3 +/-
/home/runner/wo...ageProcessor.cs 100% 89% -11%
/home/runner/wo...toResetEvent.cs 73% 68% -5%
/home/runner/wo...eiveEndpoint.cs 72% 69% -3%
/home/runner/wo...DefaultTopic.cs 89% 90% +1%
/home/runner/wo...efaultPubSub.cs 89% 91% +2%
/home/runner/wo...ueryExecutor.cs 65% 67% +2%
/home/runner/wo...tor/Mediator.cs 89% 91% +2%
/home/runner/wo...PromiseCache.cs 78% 81% +3%
/home/runner/wo...entsListener.cs 62% 66% +4%
/home/runner/wo...heMiddleware.cs 86% 92% +6%

Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@glen-84
glen-84 marked this pull request as ready for review October 9, 2026 14:48
@glen-84
glen-84 requested a lite review from Copilot October 9, 2026 14:49
@glen-84 glen-84 changed the title Stabilize Mocha saga integration tests against timing flakes [Mocha] Stabilize saga integration tests against timing flakes Oct 9, 2026

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.

Comment thread src/Mocha/test/Mocha.Sagas.Tests/IntegrationTests.cs
Comment thread src/Mocha/test/Mocha.Sagas.Tests/IntegrationTests.cs Outdated

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.

🔵 Needs a closer look

The step-through test can still observe a state change before its transaction commits, leaving a timing race unresolved.

0 open findings

2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Wait for first transition commit before publishing second event

src/​Mocha/​test/​Mocha.Sagas.Tests/​IntegrationTests.cs:71

Wait for the first transition's commit, not just State == "Triggered". The in-memory store loads the object already held by InMemorySagaStateStorage, and Saga.HandleEvent changes its State before the consumer commits. This poll can therefore pass while the first save is still in flight; the second event may finish and delete the saga before that save commits, leaving a state behind and making this test flaky. Use a commit-visible signal or snapshot before publishing the second event.

🧠 Review effort: Lite

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.

🟡 Changes recommended

One wait ignores its timeout result, so the test can proceed without confirming the first transition committed.

1 open finding

🧠 Review effort: Lite

Comment thread src/Mocha/test/Mocha.Sagas.Tests/IntegrationTests.cs Outdated

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.

🟡 Changes recommended

The step-through test exceeds the repository’s hard limit of five assertions per test.

1 open finding
1 resolved since last review

🧠 Review effort: Lite

Comment thread src/Mocha/test/Mocha.Sagas.Tests/IntegrationTests.cs
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