Conversation
WalkthroughRenames BatchInsert to InsertMany and adds DeleteMany in the generic GORM repository. Updates tests accordingly. Introduces GoMock mocks and a script to generate them. Adds make targets for testing, coverage, mocks, and git pre-push hook management. Updates pre-push script to use make test. Adds go.uber.org/mock dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Make as Makefile
participant Go as go toolchain
participant Git as Git Hook
Dev->>Make: make install-pre-push-hook
Make->>Git: Install scripts/git-pre-push.sh as pre-push (chmod +x)
Note right of Git: Hook installed
Dev->>Git: git push
Git->>Make: run "make test"
alt Tests directory exists
Make->>Go: go test ./... (with coverage when requested)
Go-->>Make: pass/fail
else No tests found
Make-->>Git: Skip test targets
end
Git-->>Dev: Allow/deny push
sequenceDiagram
autonumber
actor Svc as Service
participant Repo as Repository[T]
participant DB as GORM
rect rgb(234, 248, 255)
note over Repo,DB: New insert flow
Svc->>Repo: InsertMany(ctx, models)
Repo->>DB: Create(&models)
DB-->>Repo: error or inserted models
Repo-->>Svc: models, error
end
rect rgb(255, 240, 240)
note over Repo,DB: New delete flow
Svc->>Repo: DeleteMany(ctx, models)
Repo->>DB: Unscoped().Delete(&models)
DB-->>Repo: error or nil
Repo-->>Svc: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Makefile(1 hunks)go.mod(1 hunks)gorm_repository.go(3 hunks)gorm_repository_mock.go(1 hunks)gorm_transactor_mock.go(1 hunks)scripts/gen-mocks.sh(1 hunks)scripts/git-pre-push.sh(1 hunks)test/repository_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
gorm_repository_mock.go (1)
gorm_repository.go (1)
Specification(35-40)
🪛 checkmake (0.2.2)
Makefile
[warning] 15-15: Target body for "help" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 30-30: Target body for "test" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 4-4: Missing required phony target "all"
(minphony)
[warning] 4-4: Missing required phony target "clean"
(minphony)
[warning] 4-4: Missing required phony target "test"
(minphony)
🔇 Additional comments (5)
test/repository_test.go (1)
193-222: LGTM! Clean refactor from BatchInsert to InsertMany.The test has been properly updated to reflect the method rename, with all assertions and error messages consistently updated. The test logic and coverage remain sound.
gorm_repository_mock.go (1)
1-160: LGTM! Generated mock follows standard GoMock patterns.This is a generated file that properly mocks the Repository interface with all CRUD operations including the new InsertMany and DeleteMany methods. The structure and implementation follow standard GoMock conventions.
Makefile (3)
1-2: LGTM! Clear variable definitions.The TEST_DIR and COVER_PKG variables make the test targets more maintainable and configurable.
72-73: LGTM! Mock generation target.The new mock target provides a convenient way to regenerate mocks.
4-13: Static analysis hints are mostly pedantic.The checkmake warnings about missing "all", "clean", and "test" phony targets, and body length limits are stylistic preferences rather than functional issues. The "test" target is actually declared (line 6), so that warning is incorrect.
These can be safely ignored unless the project has specific Makefile style requirements. The current structure is clear and functional.
| test: | ||
| @echo "Running all tests..." | ||
| go test ./test/... | ||
| @if [ -d $(TEST_DIR) ]; then \ | ||
| go test $(TEST_DIR)/...; \ | ||
| else \ | ||
| echo "No tests found in $(TEST_DIR), skipping."; \ | ||
| fi | ||
|
|
||
| test-verbose: | ||
| @echo "Running all tests with verbose output..." | ||
| go test -v ./test/... | ||
| @if [ -d $(TEST_DIR) ]; then \ | ||
| go test -v $(TEST_DIR)/...; \ | ||
| else \ | ||
| echo "No tests found in $(TEST_DIR), skipping."; \ | ||
| fi | ||
|
|
||
| test-coverage: | ||
| @echo "Running all tests with coverage report..." | ||
| go test -v -cover -coverprofile=coverage.out -coverpkg=./... ./test/... | ||
| @if [ -d $(TEST_DIR) ]; then \ | ||
| go test -v -cover -coverprofile=coverage.out -coverpkg=$(COVER_PKG) $(TEST_DIR)/...; \ | ||
| else \ | ||
| echo "No tests found in $(TEST_DIR), skipping."; \ | ||
| fi | ||
|
|
||
| test-coverage-html: | ||
| @echo "Running all tests and generating HTML coverage report..." | ||
| go test -v -cover -coverprofile=coverage.out -coverpkg=./... ./test/... | ||
| go tool cover -html=coverage.out -o coverage.html | ||
| @echo "Coverage report generated: coverage.html" | ||
| @if [ -d $(TEST_DIR) ]; then \ | ||
| go test -v -cover -coverprofile=coverage.out -coverpkg=$(COVER_PKG) $(TEST_DIR)/... && \ | ||
| go tool cover -html=coverage.out -o coverage.html && \ | ||
| echo "Coverage report generated: coverage.html"; \ | ||
| else \ | ||
| echo "No tests found in $(TEST_DIR), skipping."; \ | ||
| fi | ||
|
|
||
| test-clean: | ||
| @echo "Cleaning test cache and running tests..." | ||
| go clean -testcache && go test -v ./test/... | ||
| @if [ -d $(TEST_DIR) ]; then \ | ||
| go clean -testcache && go test -v $(TEST_DIR)/...; \ | ||
| else \ | ||
| echo "No tests found in $(TEST_DIR), skipping."; \ | ||
| fi |
There was a problem hiding this comment.
Conditional guards silently skip tests when TEST_DIR is missing.
All test targets now check for TEST_DIR existence and echo a skip message if it's missing. While this prevents errors, it could mask issues if the directory is accidentally deleted, moved, or misnamed.
Consider whether the targets should fail loudly (exit with error) when TEST_DIR is expected but missing, especially in CI/CD environments where silent skips could give false confidence.
Example alternative approach:
test:
@echo "Running all tests..."
@if [ -d $(TEST_DIR) ]; then \
go test $(TEST_DIR)/...; \
else \
- echo "No tests found in $(TEST_DIR), skipping."; \
+ echo "ERROR: Test directory $(TEST_DIR) not found!"; \
+ exit 1; \
fi🧰 Tools
🪛 checkmake (0.2.2)
[warning] 30-30: Target body for "test" exceeds allowed length of 5 (6).
(maxbodylength)
🤖 Prompt for AI Agents
Makefile lines 30-70: the test targets currently silently skip when $(TEST_DIR)
is missing; change the guard so CI/automated runs fail loudly by returning a
non-zero exit status if $(TEST_DIR) is expected but absent (or introduce an
environment flag like FAIL_ON_MISSING_TEST_DIR to control behavior), update each
target to print a clear error message and exit 1 when the directory is missing,
and keep the current soft-skip behavior only when the flag explicitly allows it.
| find . -maxdepth 1 -name "*.go" \ | ||
| -not -name "*_test.go" \ | ||
| -not -name "*_mock.go" \ | ||
| -type f | while read -r file; do | ||
|
|
||
| # Check if file contains interface definitions | ||
| if grep -q "type.*interface\s*{" "$file"; then | ||
| count=$((count + 1)) | ||
|
|
||
| # Get base filename | ||
| base=$(basename "$file" .go) | ||
| output="./${base}_mock.go" | ||
|
|
||
| echo "Processing: $file" | ||
|
|
||
| # Get package name from the file | ||
| package=$(grep -m 1 "^package " "$file" | awk '{print $2}') | ||
|
|
||
| # Generate mock file | ||
| if GO111MODULE=on mockgen -source="$file" -destination="$output" -package="$package" 2>/dev/null; then | ||
| echo " ✓ Generated: $output" | ||
| success=$((success + 1)) | ||
| else | ||
| echo " ✗ Failed to generate mock for: $file" | ||
| failed=$((failed + 1)) | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
Fix counter tracking in mock generator loop
Because the while runs in a subshell (find … | while …), the count/success/failed variables are reset when the loop exits. The summary therefore always prints zeros, hiding failures. Run the loop in the current shell instead.
-find . -maxdepth 1 -name "*.go" \
- -not -name "*_test.go" \
- -not -name "*_mock.go" \
- -type f | while read -r file; do
+while IFS= read -r -d '' file; do
@@
- fi
-done
+ fi
+done < <(find . -maxdepth 1 -name "*.go" \
+ -not -name "*_test.go" \
+ -not -name "*_mock.go" \
+ -type f -print0)
1 participant
Summary by CodeRabbit