-
Notifications
You must be signed in to change notification settings - Fork 3
[test-improver] Improve tests for auth package #271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves test coverage and quality for the internal/auth package by converting manual error checking to testify assertions and adding 14 new edge case tests. The changes enhance test maintainability while increasing test coverage from approximately 85% to 95%.
Changes:
- Migrated all manual
if got != wantchecks to testify assertions (assert.Equal,require.ErrorIs,require.NoError) - Added 14 new edge case tests covering Unicode, special characters, whitespace variations, case sensitivity, and boundary conditions
- Improved test readability and failure diagnostics through idiomatic Go testing patterns
Comments suppressed due to low confidence (2)
internal/auth/header_test.go:125
- Similar to the Bearer case, this test expects a leading space when "Agent agent-id" is parsed. The implementation uses
strings.TrimPrefix(authHeader, \"Agent \")which would leave " agent-id" (one leading space). This documents potentially problematic behavior where agent IDs with leading whitespace could cause validation issues. Consider whether the implementation should trim whitespace after prefix removal.
wantAPIKey: " agent-id",
wantAgentID: " agent-id",
internal/auth/header_test.go:153
- This test expects trailing whitespace to be preserved in the extracted token. Trailing whitespace in authentication credentials is typically a source of bugs and should be trimmed. Consider whether the implementation should normalize whitespace in extracted values to prevent authentication failures due to accidental whitespace.
wantAPIKey: "my-token ",
wantAgentID: "my-token ",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wantAPIKey: " my-token", | ||
| wantAgentID: " my-token", |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected values include a leading space, but the implementation uses strings.TrimPrefix(authHeader, \"Bearer \") which removes only the prefix "Bearer " (with a single space). With input "Bearer my-token" (two spaces), the result will be " my-token" (one leading space), which matches the expectation. However, this test documents unexpected behavior - tokens/keys with leading whitespace are likely unintended and could cause authentication failures. Consider whether the implementation should trim all leading/trailing whitespace after prefix removal.
This issue also appears in the following locations of the same file:
- line 124
| wantAPIKey: " ", | ||
| wantAgentID: " ", | ||
| wantErr: nil, |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test expects whitespace-only headers to be treated as valid authentication. However, this is problematic from a security perspective - whitespace-only values should likely be rejected or treated as empty. The implementation at line 59 only checks for authHeader == \"\", allowing whitespace-only strings to be used as API keys/agent IDs, which could lead to security issues.
This issue also appears in the following locations of the same file:
- line 152
| wantAPIKey: " ", | |
| wantAgentID: " ", | |
| wantErr: nil, | |
| wantAPIKey: "", | |
| wantAgentID: "", | |
| wantErr: ErrMissingAuthHeader, |
Test Improvements: header_test.go
File Analyzed
internal/auth/header_test.gointernal/authImprovements Made
1. Better Testing Patterns ✅
if got != wantblocks withassert.Equal()require.ErrorIs()andrequire.NoError()assertandrequirepackagesrequire.ErrorIs()for proper error comparison vs simple equalityBefore (Manual Checking):
After (Testify Assertions):
2. Increased Coverage ✅
TestSanitizeForLogging: 6 → 9 test cases (+3)
"key-with-émojis-🔑""key!@#$%^&*()"TestParseAuthHeader: 4 → 11 test cases (+7)
"Bearer my-token""bearer my-token""Agent agent-id"" ""key!@#$%^&*()""Bearer my-token "TestValidateAPIKey: 4 → 8 test cases (+4)
provided="", expected="""My-Secret-Key"vs"my-secret-key""key with spaces""my-key "vs"my-key"Coverage Improvement:
3. Cleaner & More Stable Tests ✅
requirefor critical checks,assertfor comparisonsTest Execution
Branch:
test-improver/auth-header-testsAll changes are backwards compatible and additive. The test structure remains table-driven, maintaining consistency with existing patterns.
Why These Changes?
Selection Rationale
internal/auth/header_test.gowas selected as the #1 candidate from test file analysis because:go.mod, this file used NO assertionsif got != wantblocksImpact
Edge Cases Matter
The new edge case tests catch real-world scenarios:
Generated by Test Improver Workflow
Focuses on better testify usage, increased coverage, and cleaner test patterns