fix: configure slow branch test mock#505
Merged
Merged
Conversation
wangyb-A
approved these changes
Jul 3, 2026
SilanHe
approved these changes
Jul 3, 2026
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
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
Root Cause Analysis
Finding 1: Unit test mock bypassed branch execution
test_executor_returns_before_slow_branch_completes creates a mocked ExecutionState but did not configure wrap_user_function. Concurrent branch execution goes through child_handler, which calls state.wrap_user_function(...) and then invokes the returned wrapper. With an unconfigured Mock, that call returns another Mock instead of the original function wrapper, so the fast and slow branch bodies can be bypassed. In that state the slow branch may not execute its sleep path, making the test timing dependent and allowing both branches to appear completed.
The fix configures execution_state.wrap_user_function to return the original function, matching the real ExecutionState behavior used by the neighboring tests.
Finding 2: Example test assumed two snapshots stay identical
map_with_min_successful returns once min_successful is reached. The returned BatchResult is a snapshot taken by the handler at that point. The operation tree inspected later by the test can legitimately be slightly newer because already-running child operations may still finish and checkpoint after the BatchResult snapshot is built. That is why the handler result can report 6 successes while map_op.child_operations later shows 7 succeeded children.
This behavior is acceptable for min_successful concurrency. The test now keeps the important checks strict: no failed child operations, bounded successful child count, and STARTED count consistent with the operation-tree view.
Refs #405
Verification