chore(server): Increase domain package unit test coverage (asset and integrationapi) #1302
Conversation
WalkthroughThe pull request introduces several changes across multiple test files within 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
server/pkg/asset/upload_test.go (2)
10-28: Consider enhancing test coverage with additional test cases.While the current test implementation is good for basic validation, consider the following improvements:
- Convert to table-driven tests for better maintainability
- Add negative test cases (e.g., empty values, invalid UUIDs)
- Add edge cases (e.g., very large content length)
- Make time-based tests more reliable
Here's a suggested refactor:
func TestUpload_Upload(t *testing.T) { + // Define test cases + tests := []struct { + name string + upload *Upload + wantUUID string + wantFileName string + wantLength int64 + wantExpired bool + }{ + { + name: "valid upload", + upload: &Upload{ + uuid: "1", + project: NewProjectID(), + fileName: "file.test", + contentLength: 1, + expiresAt: time.Now().Add(time.Hour), + }, + wantUUID: "1", + wantFileName: "file.test", + wantLength: 1, + wantExpired: false, + }, + { + name: "expired upload", + upload: &Upload{ + uuid: "2", + project: NewProjectID(), + fileName: "expired.test", + contentLength: 0, + expiresAt: time.Now().Add(-time.Hour), + }, + wantUUID: "2", + wantFileName: "expired.test", + wantLength: 0, + wantExpired: true, + }, + // Add more test cases here + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantUUID, tt.upload.UUID()) + assert.Equal(t, tt.wantFileName, tt.upload.FileName()) + assert.Equal(t, tt.wantLength, tt.upload.ContentLength()) + assert.Equal(t, tt.wantExpired, tt.upload.Expired(time.Now())) + }) + } }
10-10: Add test function documentation.Consider adding a doc comment explaining the purpose and scope of this test function.
+// TestUpload_Upload verifies that Upload struct's getter methods +// return the expected values and correctly handles expiration checks func TestUpload_Upload(t *testing.T) {server/pkg/asset/upload_builder_test.go (1)
10-15: Consider using table-driven tests for better coverage.While the current test structure is good, consider using table-driven tests to cover more edge cases and make the tests more maintainable. This would allow testing various initialization scenarios more systematically.
Here's a suggested refactor:
func TestUploadBuilder_NewUpload(t *testing.T) { - ub := &UploadBuilder{ - u: &Upload{}, - } - assert.Equal(t, ub, NewUpload()) + tests := []struct { + name string + want *UploadBuilder + }{ + { + name: "empty upload", + want: &UploadBuilder{u: &Upload{}}, + }, + // Add more test cases here + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := NewUpload() + assert.Equal(t, tt.want, got) + }) + }server/pkg/asset/status_test.go (1)
78-80: LGTM! Consider enhancing test readability with table-driven tests.The new test case improves coverage by verifying the behavior with invalid input. The implementation is correct and follows the existing test pattern.
Consider refactoring the entire test function to use table-driven tests for better maintainability and readability. Here's a suggested approach:
func TestStatus_StatusFromRef(t *testing.T) { - sk := ArchiveExtractionStatusSkipped - p := ArchiveExtractionStatusPending - ip := ArchiveExtractionStatusInProgress - d := ArchiveExtractionStatusDone - f := ArchiveExtractionStatusFailed - - s := lo.ToPtr("skipped") - res := ArchiveExtractionStatusFromRef(s) - assert.Equal(t, &sk, res) - // ... other test cases ... + tests := []struct { + name string + input *string + expected *ArchiveExtractionStatus + }{ + { + name: "skipped status", + input: lo.ToPtr("skipped"), + expected: lo.ToPtr(ArchiveExtractionStatusSkipped), + }, + { + name: "invalid status", + input: lo.ToPtr("test"), + expected: nil, + }, + { + name: "nil input", + input: nil, + expected: nil, + }, + // ... other test cases ... + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res := ArchiveExtractionStatusFromRef(tt.input) + assert.Equal(t, tt.expected, res) + }) + } }server/pkg/asset/asset_test.go (1)
37-37: Consider adding test coverage for both flatFiles states.While the test covers the
falsecase forflatFiles, consider using table-driven tests to cover bothtrueandfalsecases for better test coverage.Here's a suggested approach:
func TestAsset_Type(t *testing.T) { + tests := []struct { + name string + flatFiles bool + }{ + {"with flat files disabled", false}, + {"with flat files enabled", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { aid := NewID() // ... other initializations ... got := Asset{ id: aid, // ... other fields ... - flatFiles: false, + flatFiles: tt.flatFiles, } // ... other assertions ... - assert.Equal(t, false, got.FlatFiles()) + assert.Equal(t, tt.flatFiles, got.FlatFiles()) }) + } }server/pkg/asset/preview_type_test.go (1)
324-326: Simplify the test implementation for better readability.The current implementation using an immediately invoked function expression (IIFE) is unnecessarily complex.
Consider simplifying the test like this:
-func TestPreviewType_Prev(t *testing.T) { - assert.Equal(t, func() *PreviewType { pt := PreviewType("image"); return &pt }(), PreviewTypeImage.Ref()) +func TestPreviewType_Prev(t *testing.T) { + expected := PreviewType("image") + assert.Equal(t, &expected, PreviewTypeImage.Ref()) +}server/pkg/asset/file_test.go (5)
71-101: LGTM with a minor suggestion for error message improvementThe conversion to table-driven tests is excellent and improves maintainability. Consider enhancing the error message to include the test case name for better debugging:
- t.Errorf("expected %v, got %v", tt.want, flatten) + t.Errorf("%s: expected %v, got %v", tt.name, tt.want, flatten)
228-255: LGTM with the same error message improvement suggestionThe table-driven test conversion is well-structured and includes good test cases. Consider the same error message enhancement:
- t.Errorf("expected %q, got %q", tt.want, result) + t.Errorf("%s: expected %q, got %q", tt.name, tt.want, result)
257-300: LGTM with suggestion for additional test caseThe Clone test is well-structured and covers the basic cases. Consider adding a test case to verify that mutations to the cloned object don't affect the original:
{ name: "clone mutations don't affect original", file: &File{ name: "test", children: []*File{{name: "child"}}, }, verify: func(t *testing.T, original, cloned *File) { cloned.name = "modified" cloned.children[0].name = "modified_child" assert.Equal(t, "test", original.name) assert.Equal(t, "child", original.children[0].name) }, },
302-313: Consider expanding test coverage and converting to table-driven formatWhile the test is functional, it could be more comprehensive. Consider:
- Converting to table-driven format for consistency with other tests
- Adding edge cases (nil file, empty files array)
- Testing more complex nested structures
func Test_FilePath(t *testing.T) { tests := []struct { name string file *File want []string }{ { name: "single file", file: &File{ files: []*File{{path: "/hello/c.txt"}}, }, want: []string{"/hello/c.txt"}, }, { name: "nil file", file: nil, want: nil, }, { name: "empty files", file: &File{files: []*File{}}, want: []string{}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert.Equal(t, tt.want, tt.file.FilePaths()) }) } }
89-89: Fix typo in test nameCorrect the spelling of "empyy" to "empty".
- name: "file object is empyy", + name: "file object is empty",server/pkg/asset/builder_test.go (1)
223-224: Add FlatFiles to MustBuild test casesThe
FlatFilesbuilder method is correctly added to the main test cases, but it's missing from theTestBuilder_MustBuildtest cases. Consider adding it there as well for consistency and complete coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
server/pkg/asset/asset_test.go(2 hunks)server/pkg/asset/builder_test.go(4 hunks)server/pkg/asset/file_test.go(4 hunks)server/pkg/asset/preview_type_test.go(3 hunks)server/pkg/asset/status_test.go(1 hunks)server/pkg/asset/upload_builder_test.go(1 hunks)server/pkg/asset/upload_test.go(1 hunks)
🔇 Additional comments (8)
server/pkg/asset/upload_test.go (1)
1-9: LGTM! Clean and minimal imports.
The package declaration and imports are appropriate for the test file.
server/pkg/asset/upload_builder_test.go (1)
1-9: LGTM! Clean and well-organized imports.
The package declaration and imports follow Go best practices with proper separation between standard library and third-party packages.
server/pkg/asset/asset_test.go (2)
50-50: LGTM!
The assertion follows the established testing pattern and is properly placed among other field verifications.
Line range hint 1-156: Consider implementing test coverage tracking.
Since this PR aims to increase test coverage, consider adding a coverage badge to the README.md and setting up coverage thresholds in the CI pipeline to maintain high test coverage over time.
Let's check if coverage tracking is already in place:
server/pkg/asset/preview_type_test.go (2)
215-215: LGTM: Clean struct initialization syntax.
The removal of trailing commas improves code consistency.
Also applies to: 223-223
281-284: LGTM: Good addition of edge case test.
Testing empty extension input improves coverage and ensures robust handling of edge cases.
server/pkg/asset/file_test.go (1)
4-4: LGTM: Good defensive programming practice!
The addition of nil object tests is a valuable enhancement to the test coverage, ensuring that the code handles nil receivers gracefully by returning appropriate zero values.
Also applies to: 31-37
server/pkg/asset/builder_test.go (1)
31-31: LGTM: Clean addition of flatFiles field
The new field follows the codebase's naming conventions and is appropriately typed.
| projectID := NewProjectID() | ||
| ubWithData := &UploadBuilder{ | ||
| u: &Upload{ | ||
| uuid: "1", | ||
| project: projectID, | ||
| fileName: "file.test", | ||
| contentLength: int64(1), | ||
| expiresAt: time.Now(), | ||
| }, | ||
| } | ||
|
|
||
| timeNow := time.Now | ||
| assert.Equal(t, ubWithData, ubWithData.UUID("1")) | ||
| assert.Equal(t, ubWithData, ubWithData.Project(projectID)) | ||
| assert.Equal(t, ubWithData, ubWithData.FileName("file.test")) | ||
| assert.Equal(t, ubWithData, ubWithData.ContentLength(int64(1))) | ||
| assert.Equal(t, ubWithData, ubWithData.ExpiresAt(timeNow())) | ||
| assert.Equal(t, ubWithData.u, ubWithData.Build()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add negative test cases for better coverage.
The test suite would benefit from negative test cases to verify proper handling of invalid inputs.
Consider adding tests for:
- Empty or invalid UUIDs
- Nil ProjectID
- Empty file names
- Negative content length
- Past expiration times
Example addition:
func TestUploadBuilder_Validation(t *testing.T) {
tests := []struct {
name string
build func() *Upload
wantErr bool
}{
{
name: "negative content length",
build: func() *Upload {
return NewUpload().
ContentLength(-1).
Build()
},
wantErr: true,
},
// Add more negative test cases
}
// ... implement test loop
}| timeNow := time.Now | ||
| assert.Equal(t, ubWithData, ubWithData.UUID("1")) |
There was a problem hiding this comment.
Fix potential test flakiness with time.Now.
Using time.Now() directly can make tests non-deterministic. Consider using a fixed timestamp for testing.
- timeNow := time.Now
+ fixedTime := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
- assert.Equal(t, ubWithData, ubWithData.ExpiresAt(timeNow()))
+ assert.Equal(t, ubWithData, ubWithData.ExpiresAt(fixedTime))Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@jasonkarel plz use util.MockNow from reearthx
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| assert.Equal(t, ubWithData, ubWithData.UUID("1")) | ||
| assert.Equal(t, ubWithData, ubWithData.Project(projectID)) | ||
| assert.Equal(t, ubWithData, ubWithData.FileName("file.test")) | ||
| assert.Equal(t, ubWithData, ubWithData.ContentLength(int64(1))) | ||
| assert.Equal(t, ubWithData, ubWithData.ExpiresAt(timeNow())) | ||
| assert.Equal(t, ubWithData.u, ubWithData.Build()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation between builder method calls.
The current test only verifies that the builder is returned. Consider validating the actual state of the Upload struct after each builder method call.
+ // Test UUID
assert.Equal(t, ubWithData, ubWithData.UUID("1"))
+ assert.Equal(t, "1", ubWithData.u.uuid)
+
+ // Test Project
assert.Equal(t, ubWithData, ubWithData.Project(projectID))
+ assert.Equal(t, projectID, ubWithData.u.project)
+
+ // Test FileName
assert.Equal(t, ubWithData, ubWithData.FileName("file.test"))
+ assert.Equal(t, "file.test", ubWithData.u.fileName)
+
+ // Test ContentLength
assert.Equal(t, ubWithData, ubWithData.ContentLength(int64(1)))
+ assert.Equal(t, int64(1), ubWithData.u.contentLength)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
ignoring this since it returns builder object.
There was a problem hiding this comment.
@jasonkarel, understood. Thanks for letting me know.
✏️ Learnings added
Learnt from: jasonkarel
PR: reearth/reearth-cms#1302
File: server/pkg/asset/upload_builder_test.go:28-33
Timestamp: 2024-11-12T05:50:03.778Z
Learning: In this codebase, it's acceptable for tests to verify that the builder object is returned without checking the internal state after each method call.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -53,6 +54,7 @@ func TestBuilder_Build(t *testing.T) { | |||
| uuid: "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx", | |||
| thread: thid, | |||
| archiveExtractionStatus: lo.ToPtr(ArchiveExtractionStatusPending), | |||
| flatFiles: false, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test case for true flatFiles value
While the current test case covers flatFiles: false, consider adding another test case where flatFiles is set to true to ensure complete coverage of this new field's behavior.
Here's a suggested additional test case:
tests := Tests{
+ {
+ name: "should create an asset with flatFiles enabled",
+ input: Input{
+ id: aid,
+ project: pid,
+ createdAt: tim,
+ createdByUser: uid,
+ fileName: "hoge",
+ size: size,
+ previewType: lo.ToPtr(PreviewTypeImage),
+ uuid: "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
+ thread: thid,
+ archiveExtractionStatus: lo.ToPtr(ArchiveExtractionStatusPending),
+ flatFiles: true,
+ },
+ want: &Asset{
+ id: aid,
+ project: pid,
+ createdAt: tim,
+ user: &uid,
+ fileName: "hoge",
+ size: size,
+ previewType: PreviewTypeFromRef(lo.ToPtr("image")),
+ uuid: "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
+ thread: thid,
+ archiveExtractionStatus: lo.ToPtr(ArchiveExtractionStatusPending),
+ flatFiles: true,
+ },
+ },Also applies to: 70-70
There was a problem hiding this comment.
the code deosnt handle if flatfiles is true/ false.
There was a problem hiding this comment.
@jasonkarel, understood. Since the code doesn't handle different behaviors when flatFiles is true or false, adding an additional test case isn't necessary. Thank you for the clarification.
✏️ Learnings added
Learnt from: jasonkarel
PR: reearth/reearth-cms#1302
File: server/pkg/asset/builder_test.go:57-57
Timestamp: 2024-11-12T05:50:23.518Z
Learning: In the `server/pkg/asset` package, the `flatFiles` field does not affect behavior based on its value, so adding test cases for different `flatFiles` values is unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
server/pkg/integrationapi/project_test.go (1)
12-15: Consider making the test more deterministic and reusableWhile the setup is functional, consider these improvements:
- Use a fixed timestamp for better test reliability
- Extract project creation into a helper function for potential reuse in other tests
+func createTestProject(t *testing.T) *project.Project { + // Use a fixed timestamp for deterministic tests + fixedTime := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) + return project.New(). + ID(project.NewID()). + Workspace(project.NewWorkspaceID()). + Name("test"). + Description("testing"). + Alias("testalias"). + UpdatedAt(fixedTime). + MustBuild() +} func Test_NewProject(t *testing.T) { - timeNow := time.Now() - p1 := project.New().ID(project.NewID()).Workspace(project.NewWorkspaceID()). - Name("test").Description("testing").Alias("testalias").UpdatedAt(timeNow).MustBuild() + p1 := createTestProject(t)server/pkg/integrationapi/comment_test.go (1)
12-20: Consider improving variable names and adding setup documentationWhile the setup is functionally correct, consider:
- Using more descriptive variable names (e.g.,
userIDinstead ofuid,userCommentinstead ofc)- Adding comments to explain the test setup and data preparation
- uid := thread.NewUserID() - iid := operator.NewIntegrationID() - authorUser := operator.OperatorFromUser(uid) - authorIntegration := operator.OperatorFromIntegration(iid) - c := thread.NewComment(thread.NewCommentID(), authorUser, "test") - cIntegration := thread.NewComment(thread.NewCommentID(), authorIntegration, "test") - authorID := c.Author().User().Ref() - authorIntegrationID := cIntegration.Author().Integration().Ref() + // Create test user and integration IDs + userID := thread.NewUserID() + integrationID := operator.NewIntegrationID() + + // Create operators for both user and integration + userOperator := operator.OperatorFromUser(userID) + integrationOperator := operator.OperatorFromIntegration(integrationID) + + // Create test comments with different author types + userComment := thread.NewComment(thread.NewCommentID(), userOperator, "test") + integrationComment := thread.NewComment(thread.NewCommentID(), integrationOperator, "test") + + // Extract author references for assertions + userAuthorID := userComment.Author().User().Ref() + integrationAuthorID := integrationComment.Author().Integration().Ref()server/pkg/integrationapi/item_export_test.go (2)
20-63: Well-structured test cases with room for enhancementThe table-driven tests are comprehensive and cover all geometry types including the nil case. Consider these enhancements:
- Add more descriptive test failure messages
- Include negative test cases for invalid geometry types
Consider this improvement for more descriptive failure messages:
- assert.Equal(t, tt.expected, result, "For input %v, expected %v but got %v", tt.input, tt.expected, result) + assert.Equal(t, tt.expected, result, "Converting geometry type: input=%v, want=%v, got=%v", tt.input, tt.expected, result)
75-76: Document the geometry string formatThe hardcoded geometry string would benefit from a comment explaining its structure and format for better maintainability.
+ // Example Point geometry in GeoJSON format str := "{\"coordinates\":[139.28179282584915,36.58570985749664],\"type\":\"Point\"}"server/pkg/integrationapi/value_test.go (2)
10-115: Consider enhancing test robustness.While the test coverage is comprehensive, consider these improvements:
- Use a test assertion library like
testify/assertfor more readable assertions and better error messages.- Add explicit nil safety checks when dereferencing pointers in the actual implementation.
Example refactor using testify:
+import "github.com/stretchr/testify/assert" func TestFromValueType(t *testing.T) { // ... test cases ... for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := FromValueType(tt.input) - if result != tt.expected { - t.Errorf("expected %v, got %v", tt.expected, result) - } + assert.Equal(t, tt.expected, result) }) } }
117-218: Consider additional edge cases and assertion improvements.The test coverage is good, but consider these enhancements:
- Use a test assertion library for more readable assertions (same as previous function).
- Add test cases for empty string input and special characters.
Example additional test cases:
tests := []struct { // ... existing test cases ... + { + name: "Empty string type", + input: value.Type(""), + expected: "", + }, + { + name: "Special characters type", + input: value.Type("@#$%"), + expected: "", + }, }server/pkg/integrationapi/asset_test.go (1)
58-119: Consider adding more test cases for better coverage.The test function has a good structure using table-driven tests. However, it could be enhanced with additional test cases to cover:
- Partial nil cases (nil asset with valid file or vice versa)
- Different file types and sizes
- Various preview types
- Empty strings for name/path
server/pkg/integrationapi/condition_test.go (2)
13-282: Consider standardizing test error messages and improving test names.While the test implementations are solid, consider these improvements for consistency and clarity:
- Standardize error messages across all operator tests (some use
assert.Equal, others uset.Errorf).- Make test names more descriptive (e.g., "equals" -> "should_convert_equals_operator").
Example improvement for test names:
{ - name: "equals", + name: "should_convert_equals_operator_successfully", input: ConditionBoolOperatorEquals, expected: view.BoolOperatorEquals, },
284-320: Maintain consistency with other test implementations.These tests differ in style from the earlier ones:
- They don't use named test cases (missing the
namefield in test structs).- They use
t.Errorfinstead ofassert.Equallike other tests.Consider aligning the implementation style with other tests in the file.
Example alignment:
tests := []struct { + name string input ConditionNullableOperator expected view.NullableOperator }{ - {Empty, view.NullableOperatorEmpty}, + { + name: "should_convert_empty_operator", + input: Empty, + expected: view.NullableOperatorEmpty, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
server/pkg/asset/upload_builder_test.go(1 hunks)server/pkg/integrationapi/asset_test.go(2 hunks)server/pkg/integrationapi/comment_test.go(1 hunks)server/pkg/integrationapi/condition_test.go(1 hunks)server/pkg/integrationapi/item_export_test.go(1 hunks)server/pkg/integrationapi/project_test.go(1 hunks)server/pkg/integrationapi/value_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/pkg/asset/upload_builder_test.go
🔇 Additional comments (13)
server/pkg/integrationapi/project_test.go (1)
1-10: LGTM: Package declaration and imports are well-organized
The package name matches the directory, and all imports are properly utilized.
server/pkg/integrationapi/comment_test.go (3)
1-11: LGTM: Well-organized imports
The imports are clean and all are being utilized in the test implementation.
55-61: LGTM: Clean test execution
The test execution follows best practices:
- Uses subtests for better error reporting
- Uses clear assertions
21-53: Consider adding more edge cases
While the current test cases cover the basic scenarios, consider adding tests for:
- Empty content
- Maximum content length (if applicable)
- Special characters in content
- Different timestamp scenarios
server/pkg/integrationapi/item_export_test.go (2)
1-18: LGTM! Well-structured test file setup
The package structure and imports are well-organized, following Go conventions and including all necessary dependencies for testing.
1-143: Verify test coverage improvement
While the tests are well-structured, let's verify that they actually increase the coverage as intended in the PR objectives.
server/pkg/integrationapi/value_test.go (2)
1-9: LGTM! Clean imports and good use of helper package.
The imports are minimal and well-organized. Good practice using the lo package for pointer operations.
1-218: Overall excellent test coverage with good structure!
The test file successfully achieves the PR objective of increasing test coverage. The table-driven tests are well-organized and cover most scenarios effectively. While there are some minor improvements suggested above, the overall quality of the tests is good.
server/pkg/integrationapi/asset_test.go (3)
54-55: LGTM! Good addition of nil input validation.
The nil check is a valuable addition that ensures proper handling of edge cases.
121-167: LGTM! Comprehensive status conversion tests.
Excellent test coverage with:
- All possible status values
- Edge cases (nil, unknown status)
- Clear test case names
- Proper value comparison
169-232: LGTM! Verify complete preview type coverage.
Excellent test coverage with comprehensive cases. Let's verify that all preview types from the asset package are covered.
✅ Verification successful
Based on the results from the shell scripts, I can now generate the final response:
All preview types are properly covered ✓
The test cases in TestToPreviewType cover all preview types defined in server/pkg/asset/preview_type.go:
- PreviewTypeImage
- PreviewTypeImageSvg
- PreviewTypeGeo
- PreviewTypeGeo3dTiles
- PreviewTypeGeoMvt
- PreviewTypeModel3d
- PreviewTypeCSV
- PreviewTypeUnknown
Plus edge cases for nil and unrecognized types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all preview types from the asset package are covered in the tests
# Find all preview type constants in the asset package
echo "Preview types defined in asset package:"
rg -A 1 "PreviewType[A-Z][a-zA-Z0-9]+" server/pkg/asset
# Compare with test coverage
echo -e "\nPreview types tested in TestToPreviewType:"
rg "PreviewType[A-Z][a-zA-Z0-9]+" server/pkg/integrationapi/asset_test.go
Length of output: 13747
server/pkg/integrationapi/condition_test.go (2)
1-11: LGTM! Clean import organization.
The imports are well-organized with standard library packages separated from third-party imports.
353-708: LGTM! Comprehensive test coverage.
The TestConditionInto function provides excellent coverage:
- Tests all condition types
- Handles complex nested conditions (AND/OR)
- Includes edge cases (nil, empty conditions)
- Uses clear test case names
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := NewProject(p1) | ||
| assert.Equal(t, tt.want, result) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect test parameter and consider parallel execution
There's a bug in the test execution where p1 is used instead of tt.p. Also, consider enabling parallel test execution.
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+ t.Parallel()
- result := NewProject(p1)
+ result := NewProject(tt.p)
assert.Equal(t, tt.want, result)
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| result := NewProject(p1) | |
| assert.Equal(t, tt.want, result) | |
| }) | |
| } | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Parallel() | |
| result := NewProject(tt.p) | |
| assert.Equal(t, tt.want, result) | |
| }) | |
| } | |
| } |
There was a problem hiding this comment.
@jasonkarel, thank you for addressing this issue!
(_/)
(^‿^)
(")_(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| Content: lo.ToPtr("test"), | ||
| CreatedAt: lo.ToPtr(cIntegration.CreatedAt()), | ||
| Id: cIntegration.ID().Ref(), | ||
| AuthorType: lo.ToPtr(Integrtaion), |
There was a problem hiding this comment.
Fix typo in AuthorType value
There's a typo in the integration author type value: Integrtaion should be Integration.
- AuthorType: lo.ToPtr(Integrtaion),
+ AuthorType: lo.ToPtr(Integration),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AuthorType: lo.ToPtr(Integrtaion), | |
| AuthorType: lo.ToPtr(Integration), |
There was a problem hiding this comment.
will fix the typo issue later as it will change the non unit testing code. the task in here is the unit testing code
There was a problem hiding this comment.
@jasonkarel, understood. If you'd like, I can open a GitHub issue to track this typo fix for later.
| if (err != nil) == tt.expectError { | ||
| fmt.Printf("Error is not matched") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix incorrect error assertion
The current error assertion logic is incorrect and only prints a message instead of failing the test.
Apply this fix:
- if (err != nil) == tt.expectError {
- fmt.Printf("Error is not matched")
- }
+ if tt.expectError {
+ assert.Error(t, err, "Expected an error but got none")
+ } else {
+ assert.NoError(t, err, "Expected no error but got: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (err != nil) == tt.expectError { | |
| fmt.Printf("Error is not matched") | |
| } | |
| }) | |
| if tt.expectError { | |
| assert.Error(t, err, "Expected an error but got none") | |
| } else { | |
| assert.NoError(t, err, "Expected no error but got: %v", err) | |
| } | |
| }) |
There was a problem hiding this comment.
@jasonkarel plz fix this one
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/pkg/integrationapi/asset_test.go (2)
58-119: Consider adding more edge cases to strengthen test coverageThe table-driven test implementation looks good and follows Go best practices. Consider adding these additional test cases to strengthen coverage:
- Partial nil cases (nil asset with valid file or vice versa)
- Empty string URL
- Zero-value time
- Invalid/empty IDs
114-116: Consider standardizing assertion methodsFor consistency, consider standardizing the assertion methods across all tests. Currently, the file mixes:
reflect.DeepEqualwith manual error messagesassert.Equalassert.NilConsider using
assertpackage consistently across all tests for better readability and consistent error messages.Also applies to: 163-165, 232-233
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
server/pkg/asset/upload_builder_test.go(1 hunks)server/pkg/integrationapi/asset_test.go(2 hunks)server/pkg/integrationapi/condition_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/pkg/asset/upload_builder_test.go
- server/pkg/integrationapi/condition_test.go
🔇 Additional comments (3)
server/pkg/integrationapi/asset_test.go (3)
54-56: LGTM: Good addition of nil case test
The addition of the nil input test case improves edge case coverage.
133-136: Use defined constants for status values
As mentioned in the previous review, please use the defined constants for status values instead of string literals.
Also applies to: 138-141, 143-146, 148-151
170-234: LGTM: Comprehensive preview type testing
Excellent implementation with:
- Complete coverage of all preview types
- Good handling of edge cases (nil, unknown, unrecognized)
- Clear test case organization
- Proper use of parallel test execution
jasonkarel
force-pushed
the
chore-increase-package-ut-coverage
branch
from
ace4825 to
b8bc793
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
server/pkg/integrationapi/asset_test.go (2)
58-119: Consider adding more test cases for better coverage.While the current implementation is good, consider adding these test cases:
- Empty strings for name/path
- Zero values for size
- Invalid URL formats
- Partial nil cases (nil asset with valid file or vice versa)
Also, consider using
assert.Equalinstead ofreflect.DeepEqualfor better error messages and consistency with other tests in the file.Example additional test cases:
tests := []struct { // ... existing fields }{ // ... existing test cases + { + name: "empty strings", + a: a, + f: asset.NewFile().Name("").Path("").ContentType("").Size(0).Build(), + url: "", + all: true, + want: &Asset{ + // ... expected values for empty strings + }, + }, + { + name: "nil asset with valid file", + a: nil, + f: f1, + url: "www.", + all: true, + want: nil, + }, }
121-168: Consider using assert.Equal for consistency.The test implementation is comprehensive and well-structured. However, for consistency with other tests in the file (like TestToPreviewType), consider using
assert.Equalinstead ofreflect.DeepEqual.- if !reflect.DeepEqual(result, tt.expected) { - t.Errorf("ToAssetArchiveExtractionStatus() expected %v, got %v", tt.expected, result) - } + assert.Equal(t, tt.expected, result)server/pkg/asset/file_test.go (4)
32-37: LGTM! Consider testing additional nil cases.Good addition of nil object tests to prevent null pointer dereferences. The tests verify that all getter methods handle nil gracefully.
Consider adding nil tests for other methods like
SetName(),AppendChild(), andIsDir()for complete coverage.
71-101: LGTM! Fix typo in test case name.Good refactoring to table-driven tests. The test cases effectively cover both success and nil scenarios.
Fix the typo in the test case name:
- name: "file object is empyy", + name: "file object is empty",
228-255: LGTM! Consider additional edge cases.Good refactoring to table-driven tests with proper nil handling.
Consider adding test cases for:
- Invalid UUID format
- Empty path in file object
- Special characters in path
257-300: LGTM! Consider adding mutation verification.Well-structured test with good coverage of both success and nil cases.
Consider adding a test case that verifies modifications to the cloned object don't affect the original:
t.Run("verify deep copy", func(t *testing.T) { original := &File{name: "test", children: []*File{{name: "child"}}} clone := original.Clone() clone.name = "modified" clone.children[0].name = "modified_child" assert.Equal(t, "test", original.name) assert.Equal(t, "child", original.children[0].name) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
server/pkg/asset/asset_test.go(2 hunks)server/pkg/asset/builder_test.go(4 hunks)server/pkg/asset/file_test.go(4 hunks)server/pkg/asset/preview_type_test.go(3 hunks)server/pkg/asset/status_test.go(1 hunks)server/pkg/asset/upload_builder_test.go(1 hunks)server/pkg/asset/upload_test.go(1 hunks)server/pkg/integrationapi/asset_test.go(2 hunks)server/pkg/integrationapi/comment_test.go(1 hunks)server/pkg/integrationapi/condition_test.go(1 hunks)server/pkg/integrationapi/item_export_test.go(1 hunks)server/pkg/integrationapi/project_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- server/pkg/asset/asset_test.go
- server/pkg/asset/builder_test.go
- server/pkg/asset/preview_type_test.go
- server/pkg/asset/status_test.go
- server/pkg/asset/upload_builder_test.go
- server/pkg/asset/upload_test.go
- server/pkg/integrationapi/comment_test.go
- server/pkg/integrationapi/condition_test.go
- server/pkg/integrationapi/item_export_test.go
- server/pkg/integrationapi/project_test.go
🔇 Additional comments (2)
server/pkg/integrationapi/asset_test.go (2)
54-55: LGTM: Good addition of nil input test case.
The added test case improves coverage by verifying the behavior with nil input.
170-234: LGTM: Well-implemented comprehensive test coverage.
The test is well-structured with:
- Comprehensive coverage of all preview types
- Proper handling of edge cases (nil, unknown types)
- Good use of parallel test execution
- Consistent assertion style
jasonkarel
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
server/pkg/asset/preview_type_test.go (1)
324-327: Simplify the test implementation for better readability.The test uses an unnecessarily complex IIFE (Immediately Invoked Function Expression). Consider simplifying it for better readability while maintaining the same test coverage.
-func TestPreviewType_Prev(t *testing.T) { - t.Parallel() - assert.Equal(t, func() *PreviewType { pt := PreviewType("image"); return &pt }(), PreviewTypeImage.Ref()) -} +func TestPreviewType_Prev(t *testing.T) { + t.Parallel() + expected := PreviewType("image") + assert.Equal(t, &expected, PreviewTypeImage.Ref()) +}server/pkg/integrationapi/asset_test.go (1)
57-117: Consider adding more test cases for better coverageWhile the current test cases cover basic success and nil scenarios, consider adding these cases to improve coverage:
- Asset without file
- File without asset
- Empty URL
- Different preview types
- Different content types
Example additional test cases:
tests := []struct { // ... existing fields }{ // ... existing test cases + { + name: "asset without file", + a: a, + f: nil, + url: "www.", + all: true, + want: &Asset{ + Id: a.ID(), + Url: "www.", + CreatedAt: timeNow, + TotalSize: lo.ToPtr(float32(100)), + ProjectId: pid, + }, + }, + { + name: "empty url", + a: a, + f: f1, + url: "", + all: true, + want: &Asset{ + // ... expected fields with empty URL + }, + }, }server/pkg/asset/file_test.go (3)
70-99: Fix typo in test nameThe refactoring to table-driven tests is excellent and improves test maintainability. However, there's a typo in the test name "empyy" which should be "empty".
- name: "file object is empyy", + name: "file object is empty",
254-296: Consider adding mutation verification testThe test implementation is well-structured and covers both success and nil cases. Consider adding a test case that verifies the independence of the cloned object by mutating the original and ensuring the clone remains unchanged.
Example test case to add:
{ name: "verify independent clone", file: &File{ name: "test", children: []*File{{name: "child"}}, }, verify: func(t *testing.T, original, cloned *File) { original.name = "modified" original.children[0].name = "modified_child" assert.Equal(t, "test", cloned.name) assert.Equal(t, "child", cloned.children[0].name) }, },
298-310: Consider expanding test cases using table-driven testsWhile the current test case is valid, consider refactoring to a table-driven test to include additional cases such as:
- nil file object
- empty files array
- nested file structures
- files with special characters in paths
Example structure:
tests := []struct { name string file *File want []string }{ { name: "success with single file", file: &File{ name: "hello.zip", files: []*File{{path: "/hello/c.txt"}}, }, want: []string{"/hello/c.txt"}, }, { name: "nil file object", file: nil, want: nil, }, { name: "empty files array", file: &File{files: []*File{}}, want: []string{}, }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
server/pkg/asset/file_test.go(3 hunks)server/pkg/asset/preview_type_test.go(3 hunks)server/pkg/asset/upload_test.go(1 hunks)server/pkg/integrationapi/asset_test.go(2 hunks)server/pkg/integrationapi/comment_test.go(1 hunks)server/pkg/integrationapi/item_export_test.go(1 hunks)server/pkg/integrationapi/project_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server/pkg/asset/upload_test.go
- server/pkg/integrationapi/comment_test.go
- server/pkg/integrationapi/item_export_test.go
- server/pkg/integrationapi/project_test.go
🔇 Additional comments (7)
server/pkg/asset/preview_type_test.go (1)
281-284: LGTM! Good addition of edge case testing.
The test case for empty extension handling improves coverage of boundary conditions.
server/pkg/integrationapi/asset_test.go (4)
53-54: LGTM: Good addition of nil input test case
The addition of the nil input test case improves edge case coverage.
131-149: Use defined constants for status values
As suggested in the previous review, use the defined constants for status values instead of string literals.
166-230: LGTM: Well-structured and comprehensive preview type tests
The test cases are well-organized and provide comprehensive coverage of:
- All preview types
- Nil input handling
- Unrecognized type handling
Line range hint 1-230: LGTM: Well-structured test file with good practices
The test file demonstrates several good practices:
- Consistent use of table-driven tests
- Parallel test execution
- Clear test case naming
- Good mix of happy path and edge cases
This contributes well to the PR objective of increasing test coverage.
server/pkg/asset/file_test.go (2)
30-36: Great addition of nil object handling tests!
The addition of nil object tests improves the robustness of the test suite by verifying that the File methods handle nil receivers gracefully, returning appropriate zero values.
226-252: Well-structured table-driven tests!
The refactoring improves test maintainability and readability. Good addition of the nil case and proper use of parallel test execution.
Overview
Increase unit testing coverage in asset (85% to 99%) and integrationapi (2% to 51.5%) packages
What I've done
added more ut to cover codes in asset and integrationapi packages
What I haven't done
How I tested
run package coverage test
Screenshot
Which point I want you to review particularly
test cases of asset and integrationapi packages.
Memo
Summary by CodeRabbit
New Features
nilinputs and various status conditions.Bug Fixes
Tests