General improvements to support refactoring of go-git tests#71
General improvements to support refactoring of go-git tests#71pjbgf wants to merge 10 commits intogo-git:mainfrom
Conversation
To enable the refactoring of existing go-git tests, whereby object format specific logic is hard-coded (e.g. creation of hashers), a new ObjectFormat was added to the Fixture struct. Note that even if the config of a given fixture does not contain an ObjectFormat defined, the new struct will define the intended value, which at this point is be mostly sha1. Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request refactors the go-git-fixtures library to better support SHA-256 testing and improve error handling. The changes enable dynamic testing across different object formats (SHA1 and SHA256) and replace panic-based error handling with proper error returns, providing a more robust API for consumers like go-git/cli.
Changes:
- Added
ObjectFormatfield to all fixtures to distinguish between SHA1 and SHA256 repositories - Converted
Packfile(),Idx(),Rev(),DotGit(), andWorktree()methods from panic-based to error-returning implementations - Added
ByObjectFormat()filtering function to enable object-format-specific test selection
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| fixtures.go | Added ObjectFormat field to all 39 fixtures, converted panic-based methods to return errors, added ByObjectFormat filter function, and comprehensive documentation |
| osfixture.go | Updated to match new error-returning API, added ObjectFormat to Clone method, improved documentation |
| osfixture_test.go | New test file covering OSFixture functionality with error handling |
| fixtures_test.go | Updated all tests to handle new error-returning API, added tests for ByObjectFormat and EnsureIsBare, updated test counts for new SHA256 fixtures |
| go.mod | Updated comment to reflect new Go version support policy (last 2 stable versions) |
| README.md | Updated documentation to include .rev files and fixed typo in shasum example |
| .golangci.yaml | Disabled exhaustruct linter for test files |
| .github/workflows/test.yml | Changed Go version matrix from explicit versions to oldstable/stable |
| internal/tgz/tgz_test.go | Replaced panic on error with require.NoError for better test error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file, err := Filesystem.Open(fmt.Sprintf("data/pack-%s.rev", f.PackfileHash)) | ||
| if err != nil { | ||
| panic(err) | ||
| return nil, os.ErrNotExist |
There was a problem hiding this comment.
The error handling is inconsistent between Packfile/Idx/Rev and DotGit/Worktree. When Filesystem.Open fails in Packfile(), Idx(), and Rev(), the original error is replaced with os.ErrNotExist, potentially masking other types of errors like permission or I/O errors. In contrast, DotGit() and Worktree() return the original error. Consider either preserving the original error for better diagnostics or documenting why os.ErrNotExist is always returned for these methods.
| } | ||
| file, err := f.Packfile() | ||
| require.NoError(t, err) | ||
| assert.NotNil(t, file, "failed to get pack file", i) |
There was a problem hiding this comment.
The test coverage for idx files has been reduced in TestEmbeddedFiles. The previous implementation verified that idx files could be retrieved for all fixtures with PackfileHash, with a special case to skip the thinpack (ee4fef0) which doesn't have an idx file. While TestIdx provides coverage for fixtures tagged "packfile", it doesn't test all fixtures with a PackfileHash. Consider restoring idx file validation in TestEmbeddedFiles to ensure all non-thinpack fixtures with packfiles also have idx files.
| assert.NotNil(t, file, "failed to get pack file", i) | |
| assert.NotNil(t, file, "failed to get pack file", i) | |
| // All fixtures with a packfile should also have an idx file, | |
| // except for the thinpack (ee4fef0), which intentionally has none. | |
| if f.PackfileHash != "ee4fef0" { | |
| idx, err := f.Idx() | |
| require.NoError(t, err) | |
| assert.NotNil(t, idx, "failed to get idx file", i) | |
| } |
| file, err := Filesystem.Open(fmt.Sprintf("data/pack-%s.pack", f.PackfileHash)) | ||
| if err != nil { | ||
| panic(err) | ||
| return nil, os.ErrNotExist |
There was a problem hiding this comment.
The error handling is inconsistent between Packfile/Idx/Rev and DotGit/Worktree. When Filesystem.Open fails in Packfile(), Idx(), and Rev(), the original error is replaced with os.ErrNotExist, potentially masking other types of errors like permission or I/O errors. In contrast, DotGit() and Worktree() return the original error. Consider either preserving the original error for better diagnostics or documenting why os.ErrNotExist is always returned for these methods.
| file, err := Filesystem.Open(fmt.Sprintf("data/pack-%s.idx", f.PackfileHash)) | ||
| if err != nil { | ||
| panic(err) | ||
| return nil, os.ErrNotExist |
There was a problem hiding this comment.
The error handling is inconsistent between Packfile/Idx/Rev and DotGit/Worktree. When Filesystem.Open fails in Packfile(), Idx(), and Rev(), the original error is replaced with os.ErrNotExist, potentially masking other types of errors like permission or I/O errors. In contrast, DotGit() and Worktree() return the original error. Consider either preserving the original error for better diagnostics or documenting why os.ErrNotExist is always returned for these methods.
go-git-fixturesplay a significant role ingo-gittesting, and now that the latter supportsSHA256, the former needs to be amended so that such tests can be done more effectively.Fixtures now have a
ObjectFormatfield, enabling dynamic tests across the supported object formats, as opposed to the current hard coded tests which are object format specific. As more SHA256 fixtures are added,go-gittests can be refactored so that they cover both without code duplication.Calls to
Packfile(),Idx()andRev()no longer lead topanic, but instead return errors. This makes the code slightly more verbose, but conversily provide a better API for things likego-git/cli.Relates to go-git/go-git#706.