Skip to content

test: fix testSendReceiveTwo and testBatchNack#270

Open
roseyang62 wants to merge 60 commits intosalesforce:mainfrom
roseyang62:investigate-pubsub-conformance-test
Open

test: fix testSendReceiveTwo and testBatchNack#270
roseyang62 wants to merge 60 commits intosalesforce:mainfrom
roseyang62:investigate-pubsub-conformance-test

Conversation

@roseyang62
Copy link
Contributor

@roseyang62 roseyang62 commented Jan 24, 2026

Summary

This PR fixes flaky PubSub testSendReceiveTwo and testBatchNack conformance tests that were timing out in CI intermittently.

Some conventions to follow

  1. add the module name as a prefix
    • for example: add a prefix: docstore: for document store module, blobstore for Blob Store module
  2. for a test only PR, add test:
  3. for a perf improvement only PR, add perf:
  4. for a refactoring only PR, add "refactor:"

@roseyang62 roseyang62 marked this pull request as draft January 24, 2026 03:20
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.25%. Comparing base (03f6a8c) to head (4caf0fc).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #270      +/-   ##
============================================
- Coverage     82.36%   82.25%   -0.12%     
  Complexity      625      625              
============================================
  Files           191      191              
  Lines         11503    11503              
  Branches       1525     1525              
============================================
- Hits           9475     9462      -13     
- Misses         1357     1370      +13     
  Partials        671      671              
Flag Coverage Δ
unittests 82.25% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@roseyang62 roseyang62 changed the title fix: enable pubsub batchNack fix: fixtestSendReceiveTwo Jan 25, 2026
@roseyang62 roseyang62 changed the title fix: fixtestSendReceiveTwo test: fix testSendReceiveTwo Jan 25, 2026
@roseyang62 roseyang62 marked this pull request as ready for review January 29, 2026 15:53
@roseyang62 roseyang62 changed the title test: fix testSendReceiveTwo test: fix testSendReceiveTwo and testBatchNack Jan 29, 2026
Comment on lines +444 to +450
// replay mode: An exception is thrown even when the timeout has not occurred.
String wireMockError = TestsUtil.getWireMockUnmatchedRequestsError();
String errorMsg = String.format(
"Failed to receive messages: Got exception after receiving %d/%d messages.%n%n" +
"WireMock Error Details:%n%s",
received.size(), expectedCount, wireMockError != null ? wireMockError : "No unmatched requests");
throw new AssertionError(errorMsg, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bringing WireMock into the test logic crosses the intended abstraction boundary. We should keep it only setup phase as in the current code and ensure the tests remain independent of the record/replay mechanism.

Copy link
Contributor

@sandeepvinayak sandeepvinayak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code breaks our design principle test shouldn't care/know if it's record/replay and is there is a wiremock in between.

@roseyang62 roseyang62 force-pushed the investigate-pubsub-conformance-test branch from f080175 to f33c0da Compare February 26, 2026 17:12
@roseyang62 roseyang62 marked this pull request as draft February 26, 2026 22:54
@roseyang62 roseyang62 force-pushed the investigate-pubsub-conformance-test branch from ffcf472 to 0facde9 Compare February 27, 2026 17:48
@roseyang62 roseyang62 marked this pull request as ready for review March 2, 2026 20:18
@roseyang62
Copy link
Contributor Author

Brief analysis of the root cause.

  • Root Cause Analysis

The issue is related to how scenarios are handled in WireMock.
In AckMatcherRelaxingTransformer, we removed the scenario-related fields (scenarioName, requiredScenarioState, newScenarioState) from the stub mappings. As a result, the three pull stubs became unordered, independent stubs with identical request patterns. During replay, WireMock would match the same stub repeatedly, causing message duplication and out-of-order delivery.

  • Current Status

After fixing, all recent CI runs have passed. I am not confident whether this fully resolves the underlying issue or simply reduces the likelihood of failure.
I have kept the additional debug print statements in the test. If the flakiness reappears in the future, it would indicate that this approach does not fully solve the problem and further investigation will be needed.

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.

3 participants