Replace errors.Is with IsNotFoundError utility function across handlers and tests for improved readability and consistency. Remove redundant errors imports. - #112
Conversation
…for `NotFoundError` and `ValidationError`, and add corresponding tests
…ndlers and tests for improved readability and consistency. Remove redundant `errors` imports.
📝 WalkthroughWalkthroughIntroduces shared helpers for detecting wrapped not-found and validation errors, migrates HTTP handlers and service tests to use them, and removes obsolete ChangesError classification migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/capability/service_capability_test.go (1)
161-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix copy-paste error in test case.
The test case name
"Flow Not Found - Empty Name"and its mock setup do not match the expected error"product not found with ID: 999". It appears to be a copy-paste artifact of the preceding test case. Consider updating the test case to actually test a missing flow or empty name, or remove it if redundant. As per path instructions, "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/capability/service_capability_test.go` around lines 161 - 176, Correct the inconsistent test case around the createCapabilityRequest and mockCapabilityQuerier setup: either rename and adjust it to cover the intended missing-flow or empty-name scenario, or remove it if redundant. Ensure the request, mock behavior, expected error, and test name consistently validate the same behavior, while preserving coverage for the product-not-found path elsewhere.Source: Path instructions
🧹 Nitpick comments (1)
internal/errors_test.go (1)
67-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
TestValidationErrorto complete coverage.While the unit test code assesses sufficient code coverage for the error helpers, there is no test verifying the
Error()output forValidationErrorlike there is forNotFoundError. Adding it would complete the coverage parity. As per path instructions, "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".💡 Proposed optional refactor to add the test
t.Run("Error message", func(t *testing.T) { expected := "Product not found with ID: 123" if err.Error() != expected { t.Errorf("expected %q, got %q", expected, err.Error()) } }) } + +func TestValidationError(t *testing.T) { + err := NewValidationErr("invalid input") + expected := "invalid input" + if err.Error() != expected { + t.Errorf("expected %q, got %q", expected, err.Error()) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/errors_test.go` around lines 67 - 77, Add a TestValidationError test alongside TestNotFoundError in internal/errors_test.go. Construct a ValidationError with representative input, then assert its Error() output exactly matches the expected validation-error message, preserving the existing subtest style.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/capability/service_capability_test.go`:
- Around line 161-176: Correct the inconsistent test case around the
createCapabilityRequest and mockCapabilityQuerier setup: either rename and
adjust it to cover the intended missing-flow or empty-name scenario, or remove
it if redundant. Ensure the request, mock behavior, expected error, and test
name consistently validate the same behavior, while preserving coverage for the
product-not-found path elsewhere.
---
Nitpick comments:
In `@internal/errors_test.go`:
- Around line 67-77: Add a TestValidationError test alongside TestNotFoundError
in internal/errors_test.go. Construct a ValidationError with representative
input, then assert its Error() output exactly matches the expected
validation-error message, preserving the existing subtest style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 160fb5ad-26fa-430e-9f1f-631ec6205c51
📒 Files selected for processing (10)
internal/capability/handler.gointernal/capability/service_capability_test.gointernal/errors.gointernal/errors_test.gointernal/flow/handler.gointernal/platform/handler.gointernal/platform/service_test.gointernal/product/handler.gointernal/product/service_test.gointernal/response_test.go
💤 Files with no reviewable changes (1)
- internal/response_test.go
Description
Code Rabbit Summary
Summary by CodeRabbit
Bug Fixes
Tests
Fixes
Closes #