Conversation
- Added comprehensive tests for `CursorToOffset`, `RecordIOReader`, `PrefixSum64Hash`, and `NewOSFileWriterFactory` in `sstable` package. - Enhanced `shard_test.go` with better error handling and additional test cases. - Introduced sorting tests in `interface_test.go` for `Entries` using custom comparison logic. - Improved test example clarity and expanded coverage for `Entry` functionality, including `WriteTo` and `Size` methods.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances test coverage across the sstable and shard packages by adding new example tests and improving existing ones.
- Adds comprehensive example tests for record I/O, cursor iteration, entry methods, prefix-sum hashing, and OS file writer factory.
- Renames several Go example functions in
index_test.goandheader_test.go, and expands sorting tests ininterface_test.go. - Updates error-handling patterns in
shard_test.go(suppressing errors via_ =and wrappingRemoveAll).
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go/sstable/recordio_test.go | New ExampleNewRecordIOReader example |
| go/sstable/index_test.go | Renamed multiple example functions (e.g., Example_indexBufferWrite) |
| go/sstable/header_test.go | Renamed header example functions (e.g., Example_headerMarshalBinary) |
| go/sstable/entry_test.go | Added ExampleEntry_WriteTo and ExampleEntry_Size |
| go/sstable/cursor_test.go | Added ExampleCursorToOffset |
| go/sort/interface_test.go | Added ExampleEntries_sort for custom sort of Entries |
| go/shard/shard_test.go | Improved defer/Close error handling; added examples for PrefixSum64Hash.Sum64 and NewOSFileWriterFactory |
Comments suppressed due to low confidence (9)
go/sstable/index_test.go:10
- [nitpick] Go example functions should follow the
ExampleType_MethodorExampleFunctionnaming convention without a leading underscore afterExample. Consider renaming toExampleIndexBuffer_WriteorExampleIndexBufferWritefor clarity.
func Example_indexBufferWrite() {
go/sstable/index_test.go:23
- [nitpick] Rename this example to
ExampleIndexEntry_IndexOforExampleIndexEntryIndexOfto match Go's example naming conventions and ensure it’s picked up bygo testtooling.
func Example_indexEntryIndexOf() {
go/sstable/index_test.go:42
- [nitpick] Consider renaming to
ExampleIndex_ReadFromorExampleIndexReadFromso the example matches theReadFrommethod onindexand adheres to Go example conventions.
func Example_indexReadFrom() {
go/sstable/index_test.go:59
- [nitpick] Example functions should map clearly to the API under test; consider
ExampleIndex_ReadAtorExampleIndexReadAtinstead of the lowercase suffix approach.
func Example_indexReadAt() {
go/sstable/index_test.go:76
- [nitpick] To ensure
go testpicks this up and for consistency, rename toExampleIndex_WriteToorExampleIndexWriteTo.
func Example_indexWriteTo() {
go/sstable/index_test.go:95
- [nitpick] Use
ExampleIndexEntry_MarshalBinaryorExampleIndexEntryMarshalBinaryto match Go example naming conventions and link the example toMarshalBinary.
func Example_indexEntryMarshalBinary() {
go/sstable/index_test.go:113
- [nitpick] Rename to
ExampleIndexEntry_UnmarshalBinaryorExampleIndexEntryUnmarshalBinaryto align with standard Go example naming.
func Example_indexEntryUnmarshalBinary() {
go/sstable/header_test.go:8
- [nitpick] Consider renaming to
ExampleHeader_MarshalBinaryorExampleHeaderMarshalBinaryso the example is recognized and linked to theheader.MarshalBinarymethod.
func Example_headerMarshalBinary() {
go/sstable/header_test.go:21
- [nitpick] For consistency and discoverability, rename to
ExampleHeader_UnmarshalBinaryorExampleHeaderUnmarshalBinary.
func Example_headerUnmarshalBinary() {
| } | ||
|
|
||
| func ExampleNewOSFileWriterFactory() { | ||
| tempDir, err := ioutil.TempDir("", "examplefactory") |
There was a problem hiding this comment.
The ioutil package is deprecated; consider using os.MkdirTemp to create a temporary directory in newer Go versions.
| for i := 0; i < numFiles; i++ { | ||
| fileName := fmt.Sprintf("%s%05d-of-%05d", factoryPrefix, i, numFiles) | ||
| filePath := path.Join(tempDir, fileName) | ||
| content, err := ioutil.ReadFile(filePath) |
There was a problem hiding this comment.
Since ioutil is deprecated, consider replacing ioutil.ReadFile with os.ReadFile.
Suggested change
| content, err := ioutil.ReadFile(filePath) | |
| content, err := os.ReadFile(filePath) |
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
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.
CursorToOffset,RecordIOReader,PrefixSum64Hash, andNewOSFileWriterFactoryinsstablepackage.shard_test.gowith better error handling and additional test cases.interface_test.goforEntriesusing custom comparison logic.Entryfunctionality, includingWriteToandSizemethods.