feat: Improve test coverage and refactor for testability #12
Conversation
This commit introduces a significant refactoring of the `cmd` package to improve testability and increases test coverage across the application. Key changes include: - Refactored Cobra commands to use `RunE` for better error handling and testing. - Extracted business logic from command handlers into separate, testable functions. - Added comprehensive unit tests for the `cmd`, `compress`, `github`, `logger`, and `pwa` packages. - Added tests for missing command-line arguments, as requested. - Implemented the `borg all` command to clone all public repositories for a GitHub user or organization. - Restored and improved the `collect pwa` functionality. - Removed duplicate code and fixed various bugs.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughReplaces many inline Cobra command variables with New*/Get* constructors and RunE handlers; introduces URL-based "all" collection aggregating cloned repos into a single DataNode with format/compression flags; refactors GitHub, release, PWA and website packages for testability and adds extensive unit tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant CmdCtor as New*Cmd()
participant Handler as RunE
participant GitHub
participant VCS
participant PWA
participant Serializer as Tar/Matrix+Compressor
User->>CLI: run "collect all [url]" or other subcommand
CLI->>CmdCtor: construct command
CLI->>Handler: invoke RunE
alt URL -> GitHub owner flow
Handler->>GitHub: GetPublicRepos(owner)
GitHub-->>Handler: repo clone URLs
loop per repo
Handler->>VCS: CloneGitRepository(repoURL, progressWriter)
VCS-->>Handler: *datanode.DataNode
Handler->>Handler: aggregate into master DataNode
end
else PWA/Website flow
Handler->>PWA: FindManifest / DownloadAndPackagePWA
PWA-->>Handler: *datanode.DataNode
end
Handler->>Serializer: choose format -> to tar
Serializer->>Serializer: apply compression (none/gz/xz)
Serializer-->>Handler: write output file
Handler-->>CLI: return error or nil
CLI-->>User: show result or error
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)pkg/github/release_test.go (2)
cmd/collect_github_release_subcommand.go (2)
🔇 Additional comments (2)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
cmd/all.go(1 hunks)cmd/collect.go(1 hunks)cmd/collect_github.go(1 hunks)cmd/collect_github_release_subcommand.go(2 hunks)cmd/collect_github_repo.go(1 hunks)cmd/collect_pwa.go(1 hunks)cmd/collect_pwa_test.go(1 hunks)cmd/collect_website.go(4 hunks)cmd/collect_website_test.go(1 hunks)cmd/main_test.go(0 hunks)cmd/root_test.go(1 hunks)main.go(1 hunks)pkg/compress/compress_test.go(1 hunks)pkg/github/github.go(2 hunks)pkg/github/github_test.go(1 hunks)pkg/github/release.go(2 hunks)pkg/github/release_test.go(1 hunks)pkg/logger/logger_test.go(1 hunks)pkg/mocks/mock_vcs.go(1 hunks)pkg/pwa/pwa.go(3 hunks)pkg/pwa/pwa_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- cmd/main_test.go
🧰 Additional context used
🧬 Code graph analysis (16)
cmd/collect.go (1)
cmd/root.go (1)
RootCmd(24-24)
cmd/collect_website.go (2)
pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
cmd/collect_pwa_test.go (3)
cmd/root.go (1)
NewRootCmd(11-21)cmd/collect.go (1)
NewCollectCmd(18-20)cmd/collect_pwa.go (1)
NewCollectPWACmd(21-51)
pkg/github/release_test.go (2)
pkg/github/release.go (6)
ParseRepoFromURL(65-92)NewClient(17-19)GetLatestRelease(29-36)DefaultClient(25-25)DownloadReleaseAsset(39-62)NewRequest(21-23)pkg/mocks/http.go (1)
NewMockClient(68-80)
pkg/github/github_test.go (2)
pkg/mocks/http.go (1)
NewMockClient(68-80)pkg/github/github.go (1)
NewAuthenticatedClient(31-40)
pkg/mocks/mock_vcs.go (2)
pkg/datanode/datanode.go (1)
DataNode(16-18)cmd/collect_github_repo.go (1)
GitCloner(18-18)
cmd/all.go (11)
pkg/github/release.go (1)
ParseRepoFromURL(65-92)pkg/github/github.go (1)
GithubClient(19-21)cmd/collect_github_repos.go (1)
GithubClient(12-12)pkg/ui/non_interactive_prompter.go (1)
NewNonInteractivePrompter(25-30)pkg/ui/quote.go (1)
GetVCSQuote(101-110)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/ui/progress_writer.go (1)
NewProgressWriter(11-13)pkg/vcs/git.go (1)
NewGitCloner(19-21)pkg/datanode/datanode.go (1)
New(21-23)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
pkg/pwa/pwa.go (1)
pkg/datanode/datanode.go (2)
DataNode(16-18)New(21-23)
cmd/collect_github_release_subcommand.go (3)
pkg/logger/logger.go (1)
New(10-18)pkg/datanode/datanode.go (1)
New(21-23)pkg/github/release.go (3)
ParseRepoFromURL(65-92)GetLatestRelease(29-36)DownloadReleaseAsset(39-62)
cmd/collect_github_repo.go (7)
pkg/ui/non_interactive_prompter.go (1)
NewNonInteractivePrompter(25-30)pkg/ui/quote.go (1)
GetVCSQuote(101-110)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/ui/progress_writer.go (1)
NewProgressWriter(11-13)pkg/vcs/git.go (1)
GitCloner(14-16)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
cmd/root_test.go (1)
cmd/root.go (2)
Execute(28-31)NewRootCmd(11-21)
pkg/pwa/pwa_test.go (1)
pkg/pwa/pwa.go (1)
NewPWAClient(23-25)
cmd/collect_pwa.go (4)
pkg/pwa/pwa.go (2)
PWAClient(17-20)NewPWAClient(23-25)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
main.go (2)
cmd/root.go (2)
RootCmd(24-24)Execute(28-31)pkg/logger/logger.go (1)
New(10-18)
cmd/collect_website_test.go (3)
cmd/root.go (1)
NewRootCmd(11-21)cmd/collect.go (1)
NewCollectCmd(18-20)cmd/collect_website.go (1)
NewCollectWebsiteCmd(89-91)
pkg/compress/compress_test.go (1)
pkg/compress/compress.go (2)
Compress(12-40)Decompress(43-64)
🔇 Additional comments (8)
cmd/collect_github.go (1)
18-20: Accessor matches the new constructor patternReturning the shared
collectGithubCmdvia an exported helper keeps the CLI wiring consistent with the other command constructors.cmd/collect_website_test.go (1)
8-26: Tests cover the no-argument edge caseNice job asserting the Cobra error for missing arguments and ensuring the constructor stays wired.
pkg/logger/logger_test.go (1)
12-64: Good coverage of verbose vs non-verbose loggingRedirecting
os.Stderrto assert the emitted levels gives solid confidence that the handler honouring verbosity works as intended.pkg/github/github.go (1)
30-47: Injectable authenticated client looks goodMaking the HTTP client factory overridable keeps production behaviour intact while letting the tests swap in mocks cleanly.
pkg/github/github_test.go (1)
14-109: Comprehensive exercising of the GitHub clientThe mock transport plus pagination and error assertions give strong confidence in the refactored client behaviour.
cmd/collect_github_repo.go (3)
16-19: LGTM! Good testability pattern.The package-level
GitClonervariable provides a clean injection point for mocking in tests whilst maintaining simplicity for production use.
21-91: Excellent refactoring for testability.The constructor-based approach with
RunEerror handling and the mockableGitClonerinterface significantly improves testability. The logic is well-structured and errors are properly wrapped for context.
34-42: No action required—original review comment is incorrect.The original concern misunderstands the
NonInteractivePrompter.IsInteractive()implementation. The method is not a constant false return; instead, it dynamically checks whether stdout is connected to a terminal usingisatty. The logic is sound: the progress bar is created only when running in an interactive terminal environment, and skipped otherwise. The code at lines 38–42 is reachable and serves a legitimate purpose. No refactoring or fixes are needed.Likely an incorrect or invalid review comment.
This commit introduces a significant refactoring of the `cmd` package to improve testability and increases test coverage across the application. Key changes include: - Refactored Cobra commands to use `RunE` for better error handling and testing. - Extracted business logic from command handlers into separate, testable functions. - Added comprehensive unit tests for the `cmd`, `compress`, `github`, `logger`, and `pwa` packages. - Added tests for missing command-line arguments, as requested. - Implemented the `borg all` command to clone all public repositories for a GitHub user or organization. - Restored and improved the `collect pwa` functionality. - Removed duplicate code and fixed various bugs. - Addressed a resource leak in the `all` command. - Improved error handling in the `pwa` package. - Refactored `main.go` to remove duplicated logic. - Fixed several other minor bugs and inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/pwa/pwa.go (1)
33-38: Validate the HTTP status before parsing the page
FindManifesthappily parses whatever body it gets; a 404/500 ends up as an HTML error page and we report “manifest not found” instead of the real fetch failure. Mirror the pattern you added indownloadAndAdd: reject non-2xx responses up front and surface a clear error.resp, err := p.client.Get(pwaURL) if err != nil { return "", err } - defer resp.Body.Close() + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + resp.Body.Close() + return "", fmt.Errorf("failed to fetch %s: unexpected status %s", pwaURL, resp.Status) + } + defer resp.Body.Close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (28)
cmd/all.go(1 hunks)cmd/all_test.go(1 hunks)cmd/collect.go(1 hunks)cmd/collect_github.go(1 hunks)cmd/collect_github_release_subcommand.go(2 hunks)cmd/collect_github_release_subcommand_test.go(1 hunks)cmd/collect_github_repo.go(1 hunks)cmd/collect_github_repo_test.go(1 hunks)cmd/collect_github_repos.go(0 hunks)cmd/collect_pwa.go(1 hunks)cmd/collect_pwa_test.go(1 hunks)cmd/collect_website.go(4 hunks)cmd/collect_website_test.go(1 hunks)cmd/root.go(0 hunks)cmd/serve.go(0 hunks)main.go(1 hunks)pkg/datanode/datanode.go(2 hunks)pkg/github/github.go(2 hunks)pkg/github/github_test.go(1 hunks)pkg/github/release_test.go(1 hunks)pkg/logger/logger.go(0 hunks)pkg/pwa/pwa.go(3 hunks)pkg/pwa/pwa_test.go(7 hunks)pkg/tarfs/tarfs.go(2 hunks)pkg/ui/non_interactive_prompter.go(0 hunks)pkg/ui/progress_writer.go(1 hunks)pkg/ui/quote.go(0 hunks)pkg/website/website.go(2 hunks)
💤 Files with no reviewable changes (6)
- pkg/logger/logger.go
- cmd/serve.go
- cmd/root.go
- pkg/ui/non_interactive_prompter.go
- cmd/collect_github_repos.go
- pkg/ui/quote.go
🧰 Additional context used
🧬 Code graph analysis (17)
cmd/collect.go (1)
cmd/root.go (1)
RootCmd(23-23)
pkg/github/release_test.go (2)
pkg/github/release.go (6)
ParseRepoFromURL(65-92)NewClient(17-19)GetLatestRelease(29-36)DefaultClient(25-25)DownloadReleaseAsset(39-62)NewRequest(21-23)pkg/mocks/http.go (1)
NewMockClient(68-80)
cmd/collect_github_repo_test.go (4)
pkg/mocks/mock_vcs.go (1)
MockGitCloner(11-14)pkg/datanode/datanode.go (1)
New(21-23)cmd/collect_github_repo.go (1)
GitCloner(22-22)cmd/root.go (1)
NewRootCmd(10-20)
cmd/collect_website_test.go (5)
cmd/root.go (1)
NewRootCmd(10-20)cmd/collect.go (1)
NewCollectCmd(17-19)cmd/collect_website.go (1)
NewCollectWebsiteCmd(88-90)pkg/website/website.go (1)
DownloadAndPackageWebsite(16-16)pkg/datanode/datanode.go (2)
DataNode(16-18)New(21-23)
cmd/collect_website.go (2)
pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
pkg/github/github_test.go (2)
pkg/mocks/http.go (1)
NewMockClient(68-80)pkg/github/github.go (1)
NewAuthenticatedClient(31-40)
cmd/collect_pwa.go (4)
pkg/pwa/pwa.go (2)
PWAClient(17-20)NewPWAClient(23-25)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
pkg/website/website.go (1)
pkg/datanode/datanode.go (1)
DataNode(16-18)
pkg/pwa/pwa_test.go (1)
pkg/pwa/pwa.go (2)
PWAClient(17-20)NewPWAClient(23-25)
main.go (2)
cmd/root.go (2)
RootCmd(23-23)Execute(27-30)pkg/logger/logger.go (1)
New(8-16)
cmd/collect_pwa_test.go (5)
cmd/root.go (1)
NewRootCmd(10-20)cmd/collect.go (1)
NewCollectCmd(17-19)cmd/collect_pwa.go (2)
NewCollectPWACmd(21-51)CollectPWA(56-108)pkg/pwa/pwa.go (1)
MockPWAClient(186-190)pkg/datanode/datanode.go (1)
New(21-23)
cmd/collect_github_release_subcommand.go (3)
pkg/datanode/datanode.go (1)
New(21-23)pkg/logger/logger.go (1)
New(8-16)pkg/github/release.go (3)
ParseRepoFromURL(65-92)GetLatestRelease(29-36)DownloadReleaseAsset(39-62)
cmd/collect_github_release_subcommand_test.go (3)
pkg/mocks/http.go (1)
NewMockClient(68-80)pkg/github/release.go (2)
NewClient(17-19)DefaultClient(25-25)cmd/collect_github_release_subcommand.go (1)
GetRelease(50-135)
cmd/all.go (11)
pkg/github/github.go (1)
GithubClient(19-21)cmd/collect_github_repos.go (1)
GithubClient(12-12)pkg/ui/non_interactive_prompter.go (1)
NewNonInteractivePrompter(21-26)pkg/ui/quote.go (1)
GetVCSQuote(95-104)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/ui/progress_writer.go (1)
NewProgressWriter(10-12)pkg/vcs/git.go (1)
NewGitCloner(19-21)pkg/datanode/datanode.go (1)
New(21-23)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)pkg/github/release.go (1)
ParseRepoFromURL(65-92)
pkg/pwa/pwa.go (1)
pkg/datanode/datanode.go (2)
DataNode(16-18)New(21-23)
cmd/collect_github_repo.go (7)
pkg/vcs/git.go (2)
NewGitCloner(19-21)GitCloner(14-16)pkg/ui/non_interactive_prompter.go (1)
NewNonInteractivePrompter(21-26)pkg/ui/quote.go (1)
GetVCSQuote(95-104)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/ui/progress_writer.go (1)
NewProgressWriter(10-12)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
cmd/all_test.go (6)
pkg/mocks/http.go (1)
NewMockClient(68-80)pkg/github/github.go (1)
NewAuthenticatedClient(31-40)pkg/mocks/mock_vcs.go (1)
MockGitCloner(11-14)pkg/datanode/datanode.go (1)
New(21-23)cmd/collect_github_repo.go (1)
GitCloner(22-22)cmd/root.go (1)
NewRootCmd(10-20)
🔇 Additional comments (2)
pkg/tarfs/tarfs.go (2)
70-70: LGTM! Formatting improves consistency.The consolidation of simple methods into one-liners enhances readability and maintains a consistent code style across the file.
Also applies to: 95-97
99-100: LGTM! Explicit interface implementation is correct.Adding explicit
IsDir()andSys()method implementations properly completes theos.FileInfointerface. The return values are appropriate:IsDir()returns false for regular files, andSys()returns nil as is standard for custom implementations.
This commit introduces a significant refactoring of the `cmd` package to improve testability and increases test coverage across the application. Key changes include: - Refactored Cobra commands to use `RunE` for better error handling and testing. - Extracted business logic from command handlers into separate, testable functions. - Added comprehensive unit tests for the `cmd`, `compress`, `github`, `logger`, and `pwa` packages. - Added tests for missing command-line arguments, as requested. - Implemented the `borg all` command to clone all public repositories for a GitHub user or organization. - Restored and improved the `collect pwa` functionality. - Removed duplicate code and fixed various bugs. - Addressed a resource leak in the `all` command. - Improved error handling in the `pwa` package. - Refactored `main.go` to remove duplicated logic. - Fixed several other minor bugs and inconsistencies. - Made tests platform-independent by removing hardcoded `/dev/null` paths. - Fixed potential panics in tests by adding `nil` checks for errors. - Fixed test state leakage by using `t.Cleanup` to restore mocked package-level variables.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
cmd/all.go(1 hunks)cmd/all_test.go(1 hunks)cmd/collect_github.go(1 hunks)cmd/collect_github_release_subcommand.go(2 hunks)cmd/collect_github_repo_test.go(1 hunks)cmd/collect_pwa.go(1 hunks)cmd/collect_pwa_test.go(1 hunks)cmd/collect_website_test.go(1 hunks)pkg/github/release_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
cmd/all_test.go (6)
pkg/mocks/http.go (1)
NewMockClient(68-80)pkg/github/github.go (1)
NewAuthenticatedClient(31-40)pkg/mocks/mock_vcs.go (1)
MockGitCloner(11-14)pkg/datanode/datanode.go (1)
New(21-23)cmd/collect_github_repo.go (1)
GitCloner(22-22)cmd/root.go (1)
NewRootCmd(10-20)
cmd/collect_website_test.go (5)
cmd/root.go (1)
NewRootCmd(10-20)cmd/collect.go (1)
NewCollectCmd(17-19)cmd/collect_website.go (1)
NewCollectWebsiteCmd(88-90)pkg/website/website.go (1)
DownloadAndPackageWebsite(16-16)pkg/datanode/datanode.go (2)
DataNode(16-18)New(21-23)
cmd/collect_pwa_test.go (5)
cmd/root.go (1)
NewRootCmd(10-20)cmd/collect.go (1)
NewCollectCmd(17-19)cmd/collect_pwa.go (2)
NewCollectPWACmd(21-51)CollectPWA(56-114)pkg/pwa/pwa.go (1)
MockPWAClient(186-190)pkg/datanode/datanode.go (1)
New(21-23)
pkg/github/release_test.go (2)
pkg/github/release.go (6)
ParseRepoFromURL(65-92)NewClient(17-19)GetLatestRelease(29-36)DefaultClient(25-25)DownloadReleaseAsset(39-62)NewRequest(21-23)pkg/mocks/http.go (1)
NewMockClient(68-80)
cmd/collect_pwa.go (4)
pkg/pwa/pwa.go (2)
PWAClient(17-20)NewPWAClient(23-25)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)
cmd/all.go (11)
cmd/collect_github_repos.go (1)
GithubClient(12-12)pkg/github/github.go (1)
GithubClient(19-21)pkg/ui/non_interactive_prompter.go (1)
NewNonInteractivePrompter(21-26)pkg/ui/quote.go (1)
GetVCSQuote(95-104)pkg/ui/progressbar.go (1)
NewProgressBar(8-15)pkg/ui/progress_writer.go (1)
NewProgressWriter(10-12)pkg/vcs/git.go (1)
NewGitCloner(19-21)pkg/datanode/datanode.go (1)
New(21-23)pkg/matrix/matrix.go (1)
FromDataNode(39-46)pkg/compress/compress.go (1)
Compress(12-40)pkg/github/release.go (1)
ParseRepoFromURL(65-92)
cmd/collect_github_release_subcommand.go (3)
pkg/logger/logger.go (1)
New(8-16)pkg/datanode/datanode.go (1)
New(21-23)pkg/github/release.go (3)
ParseRepoFromURL(65-92)GetLatestRelease(29-36)DownloadReleaseAsset(39-62)
cmd/collect_github_repo_test.go (4)
pkg/mocks/mock_vcs.go (1)
MockGitCloner(11-14)pkg/datanode/datanode.go (1)
New(21-23)cmd/collect_github_repo.go (1)
GitCloner(22-22)cmd/root.go (1)
NewRootCmd(10-20)
🔇 Additional comments (17)
cmd/collect_website_test.go (1)
14-70: LGTM! Well-structured test coverage.The test suite provides comprehensive coverage of the website collection command:
- Argument validation (NoArgs test)
- Constructor validity
- Success path with mocked dependencies
- Error path with mocked failures
The tests properly use
t.TempDir()for portable output paths and restore mocked functions with defer, ensuring proper test isolation.cmd/collect_github.go (1)
17-19: LGTM! Naming now correctly reflects singleton behaviour.The function name
GetCollectGithubCmd()appropriately signals that it returns an existing singleton rather than creating a new instance, addressing the naming convention concern raised in previous reviews.cmd/collect_pwa_test.go (1)
33-61: LGTM! Tests are now platform-portable.The tests correctly use
t.TempDir()andfilepath.Join()for output paths (lines 40-41, 55-56), addressing the previous portability concern with/dev/null. The mock-based approach ensures proper test isolation without external dependencies.cmd/collect_github_repo_test.go (1)
12-52: LGTM! Platform-portable tests with proper mocking.The tests correctly use
t.TempDir()for output paths (lines 26, 47), resolving the previous/dev/nullportability issue. The mock injection pattern with defer-based cleanup ensures proper test isolation and resource management.cmd/all_test.go (1)
16-75: LGTM! Comprehensive integration tests with proper mocking.The test suite effectively validates the
allcommand's control flow:
- Success path with mocked GitHub API and Git cloner
- Error path with API failures
The dependency injection pattern (lines 24-30, 59-65) enables testing without external services, and proper cleanup via defer ensures test isolation.
cmd/collect_pwa.go (1)
56-114: LGTM! All previous review concerns addressed.The
CollectPWAfunction now includes:
- Input validation for format and compression (lines 60-65), addressing the previous concern about accepting invalid values
- Return of the final output path (line 113), fixing the issue where auto-generated filenames weren't communicated to the caller
- Proper error wrapping throughout
- Sensible default filename logic when none is provided (lines 102-107)
cmd/all.go (2)
26-136: LGTM! Previous review concerns thoroughly addressed.The
allcommand implementation now includes:
- URL-based owner parsing via
parseGithubOwner(line 32), supporting both owner-only and owner/repo URLs- File descriptor management with anonymous function and defer (lines 73-85), preventing FD exhaustion
- Repository name prefixing (line 83) to avoid filename collisions across repos
- Error handling with
RunEfor proper error propagationThe aggregation logic properly merges multiple repositories into a single DataNode and supports both datanode and matrix output formats with optional compression.
138-158: LGTM! Robust owner URL parsing with proper validation.The
parseGithubOwnerhelper function correctly:
- Attempts owner/repo parsing first as a fallback (lines 139-142)
- Falls back to owner-only URL parsing for organisation/user URLs
- Validates empty paths (line 150-152) and empty segments (line 154-156), preventing invalid owner values from reaching the GitHub API
This addresses all previous concerns about owner URL handling.
pkg/github/release_test.go (5)
21-46: LGTM! Comprehensive URL parsing coverage.The table-driven test covers a good range of URL formats and edge cases, including success paths and error conditions.
48-71: LGTM! NewClient restoration properly implemented.The test now correctly captures and restores the original
NewClientusingt.Cleanup, addressing the previous review feedback. The mock setup and assertions are sound.
73-99: LGTM! Clean happy path test.The test properly mocks the HTTP client, restores state with
defer, and validates the downloaded asset content.
100-127: LGTM! Nil error check properly implemented.The test now correctly verifies that an error is returned before attempting to access its message (lines 121-126), addressing the previous review feedback.
149-197: LGTM! Error path tests properly implemented.Both tests correctly handle error scenarios:
TestGetLatestRelease_Errorproperly checks for nil before accessing the error message (lines 170-175), addressing previous feedback.TestDownloadReleaseAsset_DoErrorcorrectly tests transport-level failures with proper cleanup.cmd/collect_github_release_subcommand.go (4)
3-15: LGTM!The imports are appropriate for the refactored command structure and extracted business logic.
18-38: LGTM!The refactoring to use
RunEimproves error handling and testability. Logger validation and error propagation are implemented correctly.
73-102: Pack mode correctly addresses past review feedback.The current implementation properly:
- Creates the output directory before writing (line 89)
- Uses the release tag as the basename for the output file (lines 92-96)
- Treats
--outputas a directory and derives a filename within itThis addresses the concerns raised in previous reviews.
103-138: Single-asset mode correctly addresses past review feedback.The implementation now properly creates the output directory before writing assets (lines 122-126), addressing the concern raised in previous reviews. Error handling and asset selection logic are well-implemented.
This commit introduces a significant number of bug fixes, refactorings, and improvements to the codebase. Key changes include: - Refactored Cobra commands to use `RunE` for better error handling and testing. - Extracted business logic from command handlers into separate, testable functions. - Added comprehensive unit tests for the `cmd`, `compress`, `github`, `logger`, and `pwa` packages. - Added tests for missing command-line arguments, as requested. - Implemented the `borg all` command to clone all public repositories for a GitHub user or organization. - Restored and improved the `collect pwa` functionality. - Removed duplicate code and fixed various bugs. - Addressed a resource leak in the `all` command. - Improved error handling in the `pwa` package. - Refactored `main.go` to remove duplicated logic. - Fixed several other minor bugs and inconsistencies. - Made tests platform-independent by removing hardcoded `/dev/null` paths. - Fixed potential panics in tests by adding `nil` checks for errors. - Fixed test state leakage by using `t.Cleanup` to restore mocked package-level variables. - Added validation for command-line flags. - Ensured output directories are created before writing files. - Fixed naming conventions for Cobra command constructors. - Consolidated tests for `ParseRepoFromURL`. - Improved test failure messages. - Added validation for release tags. - Aggregated errors during asset downloads.
This commit introduces a significant refactoring of the
cmdpackage to improve testability and increases test coverage across the application.Key changes include:
RunEfor better error handling and testing.cmd,compress,github,logger, andpwapackages.borg allcommand to clone all public repositories for a GitHub user or organization.collect pwafunctionality.