[test-improver] Improve tests for config/rules package#1890
Merged
Conversation
- Replace assert.Nil with require.Nil for fail-fast behavior on unexpected validation errors (5 locations) - Replace require.Error with require.NotNil for *ValidationError return types to avoid Go's interface-nil gotcha (5 locations) - Add notWantSubstr check in TestValidationError_Error to verify 'Suggestion:' is absent when no suggestion is set - Add edge case tests for AbsolutePath: 'C:' (2-char path too short for Windows check) and single-char path 'C' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness and coverage of the internal/config/rules test suite by updating assertions to correctly handle validators that return *ValidationError (a concrete pointer type) and by adding a few missing edge cases.
Changes:
- Replace
require.Error/assert.Nilpatterns withrequire.NotNil/require.Nilto avoid the Go “typed-nil to interface” pitfall and to fail fast on unexpected successes. - Extend
TestValidationError_Errorto assert the absence of the “Suggestion:” section whenSuggestionis empty. - Add additional boundary test cases for
AbsolutePathWindows-drive inputs ("C:"and"C").
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
File Analyzed
internal/config/rules/rules_test.gointernal/config/rulesImprovements Made
1. Better Testify Patterns:
require.NotNilfor*ValidationErrorreturn typesThe validation functions (
PortRange,TimeoutPositive,MountFormat,NonEmptyString,AbsolutePath) all return*ValidationError, not the standarderrorinterface. Usingrequire.Error(t, err)on a*ValidationErrorvalue hits Go's interface-nil gotcha: a nil*ValidationErrorpassed to a function acceptingerrorproduces a non-nil interface (type info present, value nil), sorequire.Errorwould incorrectly pass even when the function returned nil.Replaced all 5 occurrences with
require.NotNil(t, err, ...)which correctly uses reflection to detect nil pointers.2. Fail-Fast on Unexpected Success:
require.Niloverassert.NilChanged
assert.Nil(t, err, "Unexpected error")→require.Nil(t, err, "Unexpected validation error")at all 5 success-path locations. When a validation function unexpectedly returns an error,requirestops the test immediately rather than continuing (which could cause confusing nil-dereference panics or misleading follow-up failures).3. Increased Coverage:
TestValidationError_Errornegative assertionAdded
notWantSubstrfield andassert.NotContainschecks toTestValidationError_Error. The "error without suggestion" test case now explicitly verifies that the string"Suggestion:"is absent from the error message, confirming the conditional rendering inValidationError.Error()works correctly in both directions.4. Increased Coverage: Edge cases for
TestAbsolutePathAdded two missing edge cases for the
AbsolutePathvalidator:"C:""C"These cover the
len(value) >= 3boundary condition in the Windows path detection logic, which was previously untested.Test Execution
Tests cannot be run in this environment due to sandbox restrictions on binary execution. All changes are backward-compatible: no existing tests were removed or modified in logic, only improved in assertion style, with new test cases added.
Why These Changes?
rules_test.gowas selected because it contained the most non-idiomatic testify usage in the codebase: fiveassert.Nilcalls that should berequire.Nil(fail-fast), and fiverequire.Errorcalls that silently pass Go's interface-nil pitfall when used with concrete pointer types. The additional edge cases cover boundary conditions inAbsolutePaththat were previously untested.Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests