Updated: fix README.md typo - #1
Merged
Merged
Conversation
Owner
|
@vincentinttsh thanks! |
bluet
added a commit
that referenced
this pull request
Jun 1, 2025
🔍 **Investigation Results**: User's excellent question uncovered 2 critical issues: 1. **MockCommandRunner Design Flaw (Issue #1)**: - Problem: Returned `[]byte{}, nil` when no mock found (silent failures) - Solution: Return `errors.New("no mock found for command: " + name)` - Impact: Tests now correctly fail when mocks are missing (caught 2 bugs\!) 2. **YUM Missing Thread Safety (Issue #2)**: - Problem: YUM getRunner() lacked sync.Once (race condition risk) - Solution: Add `runnerOnce sync.Once` and proper Do() pattern like APT - Impact: Both APT and YUM now have consistent thread-safe architecture 3. **Early Return Optimization**: - Fix: checkRpmInstallationStatus returns early for empty package lists - Impact: Avoids unnecessary rpm --version calls when no packages to check **Test Fixes**: - Updated MockCommandRunner test expectations (empty→error) - Fixed YUM utils test to handle proper error behavior - All tests now pass with correct error handling **Why Tests Were Passing Before**: The APT Refresh test was incorrectly passing because MockCommandRunner returned empty bytes instead of errors for missing mocks. Our fix exposed this and led to proper test coverage. **Architecture Consistency**: YUM now matches APT's thread-safe CommandRunner implementation pattern. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
bluet
added a commit
that referenced
this pull request
Jun 2, 2025
…nagers (Issue #20) (#26) * feat: implement CommandRunner architecture for APT and YUM package managers (Issue #20) Complete CommandRunner pattern migration achieving architectural consistency: **Constructor Standardization:** - Rename NewPackageManagerWithRunner → NewPackageManagerWithCustomRunner - Clear pattern: NewPackageManager() for production, NewPackageManagerWithCustomRunner() for testing **APT Migration to CommandRunner:** - Replace all exec.Command calls with CommandRunner interface - Convert utility functions to methods: ParseFindOutput, getPackageStatus, runDpkgQuery - Eliminate parameter explosion through 3-4 function levels - Better encapsulation using a.getRunner() directly **YUM Consistency:** - Convert checkRpmInstallationStatus utility function to method - Apply same architectural patterns for consistency **Legacy Code Removal:** - Delete CommandBuilder files (command_builder.go, command_builder_test.go) - Clean up all CommandBuilder imports and references **Benefits Achieved:** - ✅ Automatic LC_ALL=C handling across all package managers - ✅ Simplified testing with map-based mocking vs shell scripts - ✅ Built-in interactive mode support - ✅ Consistent architecture between APT and YUM - ✅ Better encapsulation and cleaner method signatures - ✅ 100% test coverage maintained **Architecture Status:** - APT: ✅ Complete CommandRunner implementation - YUM: ✅ Complete CommandRunner implementation - Snap/Flatpak: Deferred for future work 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: add GitHub Sub-Issues REST API reference to CLAUDE.md 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: improve LC_ALL=C test robustness in CommandRunner - Replace unreliable 'echo $LC_ALL' with 'env' command for cross-platform reliability - Add proper assertions with strings.Contains() verification - Use t.Fatalf() for command execution failures - Add strings import for verification logic Addresses review feedback from Gemini Code Assist in PR #26. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address CodeRabbit review comments - add thread safety and missing mock - Add sync.Once pattern to getRunner() method for thread safety - Add missing mock command for Refresh operation in tests - Ensure APT CommandRunner tests cover all code paths 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: critical MockCommandRunner and YUM thread safety improvements 🔍 **Investigation Results**: User's excellent question uncovered 2 critical issues: 1. **MockCommandRunner Design Flaw (Issue #1)**: - Problem: Returned `[]byte{}, nil` when no mock found (silent failures) - Solution: Return `errors.New("no mock found for command: " + name)` - Impact: Tests now correctly fail when mocks are missing (caught 2 bugs\!) 2. **YUM Missing Thread Safety (Issue #2)**: - Problem: YUM getRunner() lacked sync.Once (race condition risk) - Solution: Add `runnerOnce sync.Once` and proper Do() pattern like APT - Impact: Both APT and YUM now have consistent thread-safe architecture 3. **Early Return Optimization**: - Fix: checkRpmInstallationStatus returns early for empty package lists - Impact: Avoids unnecessary rpm --version calls when no packages to check **Test Fixes**: - Updated MockCommandRunner test expectations (empty→error) - Fixed YUM utils test to handle proper error behavior - All tests now pass with correct error handling **Why Tests Were Passing Before**: The APT Refresh test was incorrectly passing because MockCommandRunner returned empty bytes instead of errors for missing mocks. Our fix exposed this and led to proper test coverage. **Architecture Consistency**: YUM now matches APT's thread-safe CommandRunner implementation pattern. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: improve MockCommandRunner error messages for better debugging Address ellipsis-dev bot feedback: Include full command with arguments in error messages instead of just command name. **Before**: "no mock found for command: apt" **After**: "no mock found for command: apt update" **Benefits**: - More precise debugging information - Easier to identify which specific command+args combination failed - Better developer experience when writing tests **Changes**: - Use cmdKey (contains full command) instead of name in error message - Update test expectations to match improved error format - Verified improvement works in practice: "apt update" vs "apt" **Real-world example**: ``` ❌ Before: no mock found for command: apt ✅ After: no mock found for command: apt update ``` Thanks to ellipsis-dev bot for the excellent suggestion\! 🤖 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: add clarifying comments for sync.Once design rationale Address potential confusion about sync.Once usage by documenting why it's necessary for defensive programming and zero-value struct support. **Added documentation:** - Struct field comments explain runnerOnce protects zero-value usage - getRunner() method comments detail the dual initialization pattern - Clear examples of production vs testing usage patterns **Design rationale:** - Production: NewPackageManager() pre-initializes runner - Testing: &PackageManager{} uses lazy initialization via sync.Once - Prevents panics on legitimate zero-value struct usage (15+ test cases) **Zero-value usage examples:** ```go pm := &apt.PackageManager{} pm.IsAvailable() // ✅ Works safely with sync.Once protection pm := &yum.PackageManager{} pm.ListInstalled() // ✅ Works safely with sync.Once protection ``` This documentation will help prevent future suggestions to remove the essential sync.Once protection pattern. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: improve command string parsing robustness in MockCommandRunner tests Add explicit error reporting for empty command strings in test setup. This defensive programming practice makes test failures more visible when test data is incorrectly configured. Changes: - Replace silent skip (if len > 0) with explicit error logging - Add continue statement to prevent array access on empty parts - Improve test maintainability by catching setup errors early All tests continue to pass with this change. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: complete CommandRunner architecture consistency (Issue #20) - Add executeCommand() pattern to APT for centralized command execution - Convert YUM ParseFindOutput from function to method for consistency - Extract APT environment variables to constants (DRY principle) - Fix hardcoded command names with proper constants - Reduce APT getRunner() calls from 17 to 7 (8 executeCommand uses) - Eliminate duplicate interactive/non-interactive logic across methods - Update architecture documentation with executeCommand pattern - All tests pass with zero regressions This achieves full architectural consistency between APT and YUM package managers, following proven patterns without over-engineering. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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.
No description provided.