Conversation
…-output-file Covers the two deferred items from P1/P2 stabilisation: - test_no_progress_suppresses_progress: verifies that passing --no-progress (options.no_progress=True) through handle_configure_from_options() sets the canonical config.no_progress=True and that config.show_progress returns False. - test_export_output_file_writes_file: verifies that passing --export json --output-file PATH writes the export to a file and does not echo the raw JSON export data to stdout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds two deferred integration tests to validate that the --no-progress flag correctly propagates to the configuration layer and that --export with --output-file writes JSON output to a file instead of stdout for brew listings. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
- P1: integration test for --no-progress done (PR #122) - P2: integration test for --export --output-file done (PR #122) - P3: lazy config init done (PR #121) - P5: CHANGELOG updated + PROJECT_REVIEW.md removed (PR #121) - Update objective note: all P0-P5 complete Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Tue Mar 31 19:58:29 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
![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:
- In both new tests, the stacked @patch decorators are used in the opposite order to the function arguments (the top-most patch corresponds to the first argument), so the parameters should be reordered or the decorators swapped to ensure each mock is wired to the intended target.
- The test_export_output_file_writes_file test calls handle_list_brews(options) without importing handle_list_brews in the test body, which will raise a NameError; add the appropriate import (mirroring the pattern used in the first new test).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both new tests, the stacked @patch decorators are used in the opposite order to the function arguments (the top-most patch corresponds to the first argument), so the parameters should be reordered or the decorators swapped to ensure each mock is wired to the intended target.
- The test_export_output_file_writes_file test calls handle_list_brews(options) without importing handle_list_brews in the test body, which will raise a NameError; add the appropriate import (mirroring the pattern used in the first new test).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 Mar 31 19:59:25 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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: 8385cca180
ℹ️ 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".
Comment on lines
+737
to
+740
| options = MagicMock() | ||
| options.brews = True | ||
| options.debug = False | ||
| options.export_format = "json" |
There was a problem hiding this comment.
Using MagicMock() for options here makes hasattr(options, "exclude_auto_updates") and hasattr(options, "only_auto_updates") evaluate true inside brew_handlers._get_and_filter_brews, so this test unintentionally enters the auto-update path and can call get_casks_with_auto_updates (network/Homebrew dependent). That introduces flakiness and can mask the stdout assertion if filtering drops "firefox"; use argparse.Namespace/SimpleNamespace with explicit boolean flags instead.
Useful? React with 👍 / 👎.
Comment on lines
+722
to
+724
| handle_configure_from_options(options) | ||
|
|
||
| cfg = _cfg_mod.get_config() |
There was a problem hiding this comment.
This test mutates the process-wide config singleton via handle_configure_from_options(options) and leaves no_progress=True set. Since runtime code (for example apps.finder._should_show_progress) reads get_config() directly, any tests running later in the same process can inherit this state and become order-dependent. Save and restore the previous value in a finally block (or patch the config singleton) to keep test isolation.
Useful? React with 👍 / 👎.
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
Adds two integration tests deferred from the P1/P2 stabilisation cycle:
test_no_progress_suppresses_progress: verifies--no-progressflows throughhandle_configure_from_options()→config.set("no_progress", True)→config.show_progress == False(covers the P1 fix for the deadui.show_progresskey)test_export_output_file_writes_file: verifies--export json --output-file PATHwrites to the file and does not print the JSON export to stdout (covers the P2--output-fileCLI flag)Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Add integration coverage for CLI progress suppression and export output file behavior.
Tests: