test: fix flaky concurrent test β mock handler's local import#135
Merged
test: fix flaky concurrent test β mock handler's local import#135
Conversation
β¦odule _get_apps_data() uses a by-value import so patching versiontracker.apps.get_applications had no effect in threads; patch _get_apps_data directly instead. Also raise join timeout 10s β 30s as a belt-and-suspenders guard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the concurrent operations end-to-end test to mock the handlerβs local _get_apps_data import directly and increases the thread join timeout to reduce flakiness under concurrency. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Instead of hardcoding
mock_app_tuplesin the test, consider reusing or extending the existingmock_applicationsfixture so that test data stays centralized and consistent. - The increased
thread.join(timeout=30)is a somewhat arbitrary magic number; consider adding a short comment explaining why 30 seconds is appropriate or extracting it to a named constant to clarify intent. - After
thread.join, you may want to assert that no threads are still alive (e.g.,assert not thread.is_alive()) to ensure timeouts are actually sufficient and detect hidden hangs early.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of hardcoding `mock_app_tuples` in the test, consider reusing or extending the existing `mock_applications` fixture so that test data stays centralized and consistent.
- The increased `thread.join(timeout=30)` is a somewhat arbitrary magic number; consider adding a short comment explaining why 30 seconds is appropriate or extracting it to a named constant to clarify intent.
- After `thread.join`, you may want to assert that no threads are still alive (e.g., `assert not thread.is_alive()`) to ensure timeouts are actually sufficient and detect hidden hangs early.Help me be more useful! Please click π or π on each comment and I'll use the feedback to improve your reviews.
π Security Analysis ReportSecurity Analysis ReportGenerated: Tue Apr 7 18:07:59 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 790ff9c286
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = versiontracker_main() | ||
| results.append(result) | ||
| # Patch the handler's local import (imported by value, so source-module mock doesn't intercept) | ||
| with mock.patch("versiontracker.handlers.app_handlers._get_apps_data", return_value=mock_app_tuples): |
There was a problem hiding this comment.
Patching versiontracker.handlers.app_handlers._get_apps_data inside each thread is racy: unittest.mock.patch stores/restores the target value per context, so when one thread exits its with block it can restore the real function while other threads are still running. In that window, another thread can call the real _get_apps_data (and trigger system_profiler), reintroducing the same timeout flake this commit is trying to fix. Patch this symbol once around thread startup/join so all workers see a stable mock for the full test duration.
Useful? React with πΒ / π.
Codecov Reportβ All modified and coverable lines are covered by tests. π’ Thoughts on this report? Let us know! |
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
1 participant
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
test_concurrent_operations_workflowwas failing locally because_get_apps_data()uses a by-value import (from versiontracker.apps import get_applications), so patchingversiontracker.apps.get_applicationshad no effect β each thread ran the realsystem_profilerscan (74 apps) and timed out in < 10s.versiontracker.handlers.app_handlers._get_apps_datadirectly so threads return immediately with mock data.thread.jointimeout 10s β 30s as an additional guard.Test plan
pytest tests/test_end_to_end_integration.py::TestEndToEndIntegration::test_concurrent_operations_workflowpasses in ~6s (was timing out)π€ Generated with Claude Code
Summary by Sourcery
Stabilize the concurrent operations end-to-end test by ensuring it uses mocked application data instead of hitting real system state and by relaxing thread join timeouts.
Bug Fixes:
Tests: