chore(server): Increase domain package unit test coverage (integrationapi part 2) #1307
Conversation
WalkthroughThis pull request introduces several new test files and functions for the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms canceled.
|
jasonkarel
force-pushed
the
chore-increase-package-ut-coverage-2
branch
from
a4af7f1
to
788ada2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
server/pkg/integrationapi/event_test.go (4)
17-66: Consider adding negative test cases.The test function is well-structured using table-driven tests and covers all operator types. However, consider enhancing it with:
- Negative test cases (e.g., invalid user ID)
- More descriptive error messages in assertions
Consider updating the error message to be more descriptive:
- t.Errorf("expected %+v but got %+v", test.want, result) + t.Errorf("%s: expected operator %+v but got %+v", test.name, test.want, result)
69-73: Consider using a more maintainable time mock approach.The hardcoded mock time (2024) might cause the test to fail in the future. Consider using a relative time or a test constant.
- mockTime := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) + mockTime := time.Now().UTC().Truncate(time.Second) // or define a package-level test constant + var testTime = time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC)
70-72: Extract test data setup into helper functions.The test data setup is complex and could be made more maintainable by moving it to helper functions.
Consider creating helper functions like:
func createTestUser(t *testing.T) *user.User { return user.New().NewID().Email("[email protected]").Name("John").MustBuild() } func createTestAsset(t *testing.T, u *user.User) *asset.Asset { return asset.New().NewID().Project(project.NewID()).Size(100).NewUUID(). CreatedByUser(u.ID()).Thread(id.NewThreadID()).MustBuild() }
79-85: Reduce code duplication in URL resolver.The URL resolver function is duplicated in the test cases. Consider extracting it to a variable.
+ testURLResolver := func(a *asset.Asset) string { + return "test.com" + } + ev := event.New[any]().ID(eID1).Timestamp(mockTime).Type(event.AssetCreate).Operator(operator.OperatorFromUser(u.ID())).Object(a).Project(&prj).MustBuild() - d1, _ := New(ev, "test", func(a *asset.Asset) string { - return "test.com" - }) - d2, _ := New(ev.Object(), "test", func(a *asset.Asset) string { - return "test.com" - }) + d1, _ := New(ev, "test", testURLResolver) + d2, _ := New(ev.Object(), "test", testURLResolver)server/pkg/integrationapi/schema_test.go (1)
18-93: Consider adding more test cases for better coverage.The test is well-structured with good use of table-driven tests and comprehensive field validation. However, consider these improvements:
- Add test cases for error scenarios
- Include test cases with nil inputs to verify robustness
- Add
t.Parallel()for concurrent test execution, similar toTestNewItemFieldChangesExample test case to add:
tests := []struct { name string args args want Model }{ + { + name: "with nil model", + args: args{ + m: nil, + sp: schemaPackage1, + lastModified: mockTime, + }, + want: Model{}, + }, // existing test cases... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
server/pkg/integrationapi/event_test.go(1 hunks)server/pkg/integrationapi/schema_test.go(1 hunks)
🔇 Additional comments (2)
server/pkg/integrationapi/event_test.go (1)
1-15: LGTM! Well-organized imports.
The package declaration and imports are clean and properly organized, including all necessary dependencies for testing.
server/pkg/integrationapi/schema_test.go (1)
1-16: LGTM! Package and imports are well-organized.
The package declaration matches the directory structure, and all imports are properly utilized.
| type args struct { | ||
| event *event.Event[any] | ||
| override any | ||
| v string | ||
| urlResolver asset.URLResolver | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| args args | ||
| want Event | ||
| wantErr error | ||
| }{ | ||
| { | ||
| name: "success", | ||
| args: args{ | ||
| event: ev, | ||
| override: ev, | ||
| v: "test", | ||
| urlResolver: func(a *asset.Asset) string { | ||
| return "test.com" | ||
| }, | ||
| }, | ||
| want: Event{ | ||
| ID: ev.ID().String(), | ||
| Type: string(ev.Type()), | ||
| Timestamp: ev.Timestamp(), | ||
| Data: d1, | ||
| Project: &ProjectIdAlias{ | ||
| ID: ev.Project().ID, | ||
| Alias: ev.Project().Alias, | ||
| }, | ||
| Operator: NewOperator(ev.Operator()), | ||
| }, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "success when override is nil", | ||
| args: args{ | ||
| event: ev, | ||
| override: nil, | ||
| v: "test", | ||
| urlResolver: func(a *asset.Asset) string { | ||
| return "test.com" | ||
| }, | ||
| }, | ||
| want: Event{ | ||
| ID: ev.ID().String(), | ||
| Type: string(ev.Type()), | ||
| Timestamp: ev.Timestamp(), | ||
| Data: d2, | ||
| Project: &ProjectIdAlias{ | ||
| ID: ev.Project().ID, | ||
| Alias: ev.Project().Alias, | ||
| }, | ||
| Operator: NewOperator(ev.Operator()), | ||
| }, | ||
| wantErr: nil, | ||
| }, | ||
| } | ||
| for _, test := range tests { | ||
| result, err := NewEventWith(test.args.event, test.args.override, test.args.v, test.args.urlResolver) | ||
| if !assert.Equal(t, result, test.want) { | ||
| t.Errorf("expected %+v but got %+v", test.want, result) | ||
| } | ||
| if !assert.Equal(t, err, test.wantErr) { | ||
| t.Errorf("expected %+v but got %+v", test.wantErr, err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error test cases.
The function returns an error, but there are no test cases verifying error conditions.
Consider adding test cases for:
- Invalid event (nil)
- Invalid URL resolver (nil)
- Any other error conditions from the
Newfunction
Example test case:
{
name: "should fail with nil event",
args: args{
event: nil,
override: nil,
v: "test",
urlResolver: testURLResolver,
},
want: Event{},
wantErr: errors.New("event cannot be nil"),
},| func TestNewItemFieldChanges(t *testing.T) { | ||
|
|
||
| fID := id.NewFieldID() | ||
| v0 := value.MultipleFrom(value.TypeBool, []*value.Value{ | ||
| value.New(value.TypeBool, false), | ||
| }) | ||
| v1 := value.MultipleFrom(value.TypeBool, []*value.Value{ | ||
| value.New(value.TypeBool, true), | ||
| }) | ||
|
|
||
| type args struct { | ||
| change item.FieldChanges | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| args args | ||
| want []FieldChange | ||
| }{ | ||
| { | ||
| name: "success", | ||
| args: args{ | ||
| change: item.FieldChanges{ | ||
| item.FieldChange{ | ||
| ID: fID, | ||
| Type: item.FieldChangeTypeAdd, | ||
| CurrentValue: value.MultipleFrom(v1.Type(), []*value.Value{v1.First()}), | ||
| PreviousValue: value.MultipleFrom(v0.Type(), []*value.Value{v0.First()}), | ||
| }, | ||
| }, | ||
| }, | ||
| want: []FieldChange{ | ||
| { | ||
| ID: fID, | ||
| Type: item.FieldChangeTypeAdd, | ||
| CurrentValue: v1.Interface(), | ||
| PreviousValue: v0.Interface(), | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| for _, test := range tests { | ||
| t.Run(string(test.name), func(t *testing.T) { | ||
| t.Parallel() | ||
| result := NewItemFieldChanges(test.args.change) | ||
| assert.Equal(t, test.want, result) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
While the test is well-structured, it could benefit from more comprehensive coverage:
- Add test cases for other field change types (Update, Remove)
- Include tests for different value types (String, Number, etc.)
- Add test cases for edge cases (nil values, empty changes)
Example additional test cases:
tests := []struct {
name string
args args
want []FieldChange
}{
+ {
+ name: "update change type",
+ args: args{
+ change: item.FieldChanges{
+ item.FieldChange{
+ ID: fID,
+ Type: item.FieldChangeTypeUpdate,
+ CurrentValue: value.New(value.TypeString, "new"),
+ PreviousValue: value.New(value.TypeString, "old"),
+ },
+ },
+ },
+ want: []FieldChange{
+ {
+ ID: fID,
+ Type: item.FieldChangeTypeUpdate,
+ CurrentValue: "new",
+ PreviousValue: "old",
+ },
+ },
+ },
+ {
+ name: "empty changes",
+ args: args{
+ change: item.FieldChanges{},
+ },
+ want: []FieldChange{},
+ },
// existing test cases...
}Committable suggestion skipped: line range outside the PR's diff.
jasonkarel
changed the title
jasonkarel
force-pushed
the
chore-increase-package-ut-coverage-2
branch
from
7e23754
to
f8b59f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
server/pkg/id/key_test.go (1)
59-68: Consider adding edge cases to the test function.The test implementation is clean and follows the established patterns. However, to ensure robust testing, consider adding the following test cases:
- nil pointer input
- empty string pointer input
Here's a suggested enhancement:
func TestKey_NewKeyFromPtr(t *testing.T) { + t.Run("should handle valid string pointer", func(t *testing.T) { str := "test-key" wantKey := Key{ key: lo.FromPtr(&str), } result := NewKeyFromPtr(&str) assert.NotNil(t, result, "Result should not be nil") assert.Equal(t, &wantKey, result, "Key value should match the input string") + }) + + t.Run("should handle nil pointer", func(t *testing.T) { + result := NewKeyFromPtr(nil) + assert.NotNil(t, result, "Result should not be nil") + assert.Equal(t, &Key{}, result, "Key should be empty for nil input") + }) + + t.Run("should handle empty string pointer", func(t *testing.T) { + empty := "" + result := NewKeyFromPtr(&empty) + assert.NotNil(t, result, "Result should not be nil") + assert.Equal(t, &Key{key: ""}, result, "Key should contain empty string") + }) }server/pkg/item/builder_test.go (3)
223-227: Consider adding edge cases and builder chain tests.While the basic test case is good, consider adding:
- Test with nil user ID
- Test method chaining with other builder methods
func TestBuilder_UpdatedByUser(t *testing.T) { + t.Run("should set user ID", func(t *testing.T) { uId := accountdomain.NewUserID() uuid := New().UpdatedByUser(&uId) assert.Equal(t, &uId, uuid.i.updatedByUser) + }) + + t.Run("should handle nil user ID", func(t *testing.T) { + uuid := New().UpdatedByUser(nil) + assert.Nil(t, uuid.i.updatedByUser) + }) + + t.Run("should work in builder chain", func(t *testing.T) { + uId := accountdomain.NewUserID() + item := New(). + NewID(). + UpdatedByUser(&uId). + Schema(id.NewSchemaID()). + Model(id.NewModelID()). + Project(id.NewProjectID()). + Thread(id.NewThreadID()). + MustBuild() + assert.Equal(t, &uId, item.UpdatedByUser()) + }) }
229-233: Consider adding edge cases and builder chain tests.Similar to the previous test, consider adding:
- Test with nil integration ID
- Test method chaining with other builder methods
func TestBuilder_UpdatedByIntegration(t *testing.T) { + t.Run("should set integration ID", func(t *testing.T) { iid := id.NewIntegrationID() uuid := New().UpdatedByIntegration(&iid) assert.Equal(t, &iid, uuid.i.updatedByIntegration) + }) + + t.Run("should handle nil integration ID", func(t *testing.T) { + uuid := New().UpdatedByIntegration(nil) + assert.Nil(t, uuid.i.updatedByIntegration) + }) + + t.Run("should work in builder chain", func(t *testing.T) { + iid := id.NewIntegrationID() + item := New(). + NewID(). + UpdatedByIntegration(&iid). + Schema(id.NewSchemaID()). + Model(id.NewModelID()). + Project(id.NewProjectID()). + Thread(id.NewThreadID()). + MustBuild() + assert.Equal(t, &iid, item.UpdatedByIntegration()) + }) }
235-239: Consider adding edge cases and builder chain tests.Similar to the previous tests, consider adding:
- Test with nil item ID
- Test method chaining with other builder methods
func TestBuilder_OriginalItem(t *testing.T) { + t.Run("should set original item ID", func(t *testing.T) { iId := id.NewItemID().Ref() b := New().OriginalItem(iId) assert.Equal(t, iId, b.i.originalItem) + }) + + t.Run("should handle nil item ID", func(t *testing.T) { + b := New().OriginalItem(nil) + assert.Nil(t, b.i.originalItem) + }) + + t.Run("should work in builder chain", func(t *testing.T) { + iId := id.NewItemID().Ref() + item := New(). + NewID(). + OriginalItem(iId). + Schema(id.NewSchemaID()). + Model(id.NewModelID()). + Project(id.NewProjectID()). + Thread(id.NewThreadID()). + MustBuild() + assert.Equal(t, iId, item.OriginalItem()) + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
server/pkg/id/id_test.go(1 hunks)server/pkg/id/key_test.go(1 hunks)server/pkg/item/builder_test.go(1 hunks)
🔇 Additional comments (2)
server/pkg/id/id_test.go (1)
62-69: LGTM! Test cases follow consistent patterns
The test cases for Tag, Group, and ItemGroup entities follow consistent patterns seen in other tests:
- Simple entities return their lowercase name as type (e.g., "tag", "group")
- Compound names use snake_case (e.g., "item_group")
server/pkg/item/builder_test.go (1)
223-239: LGTM! Good test coverage additions.
The new tests effectively cover the builder methods and follow the established patterns in the file. The tests are well-structured and use appropriate assertions.
| v := View{} | ||
| assert.Equal(t, "request", v.Type()) |
There was a problem hiding this comment.
💡 Codebase verification
The View.Type() method incorrectly returns "request" instead of "view"
The implementation in server/pkg/id/id.go confirms that View.Type() returns "request", which breaks the consistent pattern where entity types match their lowercase names (e.g., Tag returns "tag", Group returns "group"). This should be changed to return "view" instead.
server/pkg/id/id.go:246: Changefunc (View) Type() string { return "request" }tofunc (View) Type() string { return "view" }
🔗 Analysis chain
Verify the expected type for View entity
The test asserts that View.Type() returns "request", which seems incorrect as it doesn't match the entity name pattern seen in other tests. Other entities return their lowercase name as the type (e.g., Tag returns "tag", Group returns "group").
Let's verify the correct type by checking the View entity implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the View entity implementation to verify its Type() method
ast-grep --pattern 'type View struct {
$$$
}'
# Find the Type() method implementation for View
ast-grep --pattern 'func (View) Type() string {
$$$
}'
Length of output: 2711
Overview
Increase unit testing coverage in integrationapi and more (tbd) in pkg
What I've done
added more ut to cover codes in integrationapi packages
What I haven't done
How I tested
run UT coverage testing
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
integrationapipackage, covering functionality related to operators and events.Type()method across multiple entities.NewKeyFromPtrmethod to validate key creation from string pointers.Builderstruct with new tests for various builder methods.