[damccorm]

Fixes a flaky failure in InteractiveBeamComputeTest.test_compute_blocking (ValueError: All PCollections must belong to the same pipeline.) in PostCommit Python Versions (run 42902232829).

Root Cause

In InteractiveBeamComputeTest.test_async_computation_result_cancel, ib.compute(pcoll, blocking=False) launched a real background thread in RecordingManager._executor running time.sleep(100) on DirectRunner, while the test exited after ~1.1s without waiting for the background thread to finish. Then, the leaked background thread from test_async_computation_result_cancel continued executing during the subsequent test_compute_blocking test and mutated test_compute_blocking's InteractiveEnvironment / pipeline tracker.

Changes

• In InteractiveBeamComputeTest.test_async_computation_result_cancel, mock RecordingManager._execute_pipeline_fragment with side_effect=lambda *args: time.sleep(2) (matching test_compute_multiple_async below it) and call async_result.wait_for_completion(timeout=5) at the end of the test so no background thread or 100s pipeline execution leaks into subsequent tests.

Verification

• Triggered beam_PostCommit_Python_Versions.yml workflow run on branch: 43090965770

[damccorm]

/gemini review

[github-actions]

📋 Review Summary

This PR cleanly resolves a test flake in InteractiveBeamComputeTest caused by test_async_computation_result_cancel leaking a 100s background execution thread into subsequent tests. By synchronizing on _execute_pipeline_fragment with threading.Event and ensuring isolated_env.setUp initializes return_value atomically when patching ie.current_env, cross-test state contamination is prevented.

🔍 General Feedback

• The synchronization pattern using started_event and release_event with cleanup in a finally block makes the cancellation test deterministic and eliminates the arbitrary time.sleep delays.
• Guarding self._cancel_button against None in AsyncComputationResult.cancel() properly handles non-IPython environments when IS_IPYTHON is False.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

[damccorm]

/gemini review

[github-actions]

📋 Review Summary

This PR resolves a flaky test failure in InteractiveBeamComputeTest caused by test_async_computation_result_cancel leaking a 100-second background thread into subsequent tests. The fix cleanly replaces the time.sleep(100) pipeline execution with a synchronized mock of _execute_pipeline_fragment that is properly joined in a finally block, while also closing a brief mock initialization window in isolated_env.setUp and guarding _cancel_button access when IS_IPYTHON is false.

🔍 General Feedback

• Using threading.Event primitives (started_event and release_event) paired with wait_for_completion in finally makes test_async_computation_result_cancel deterministic and eliminates cross-test interference.
• Passing return_value=self.current_env directly to patch(...) in mock_env.py prevents ie.current_env() from briefly returning an unconfigured MagicMock during setUp.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.15%. Comparing base (0a963fc) to head (df61ac5).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #41207      +/-   ##
============================================
- Coverage     59.25%   59.15%   -0.11%     
  Complexity     4894     4894              
============================================
  Files          1461     1461              
  Lines        186046   186051       +5     
  Branches       3648     3648              
============================================
- Hits         110244   110059     -185     
- Misses        72445    72635     +190     
  Partials       3357     3357              

Flag	Coverage Δ
python	80.54% <ø> (-0.23%)	⬇️

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

☔ View full report in Codecov by Harness.
📢 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[damccorm]

/gemini review

[github-actions]

📋 Review Summary

This PR resolves a test flake in InteractiveBeamComputeTest where test_async_computation_result_cancel leaked a background pipeline execution thread running time.sleep(100) into subsequent tests. By mocking RecordingManager._execute_pipeline_fragment with a short sleep and awaiting completion before the test exits, the change cleanly eliminates cross-test pollution of the global InteractiveEnvironment.

🔍 General Feedback

• The root cause analysis and fix are spot-on. Mocking _execute_pipeline_fragment and explicitly calling wait_for_completion ensures deterministic cleanup of the background worker thread before isolated_env tears down the test environment.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.