Improve test quality: scripts/lint_error_messages_test.go#23706
Improve test quality: scripts/lint_error_messages_test.go#23706
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d351260f-2bbe-4728-ba3a-d5166fa126bd Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add testify imports (assert, require) replacing bare t.Errorf patterns - Replace complex conditional in TestCheckErrorQuality with assert.Nil/NotNil - Replace t.Errorf in TestShouldSkipQualityCheck with assert.Equal - Update TestSuggestImprovement to use assert.NotEmpty and assert.Contains to verify wantContain field - Expand TestPatternMatching from 4 cases (3 patterns) to 22 cases covering all 7 regex patterns with both matching and non-matching cases - Add TestAnalyzeFile with 5 subtests covering compliant, non-compliant, mixed, empty, and invalid file scenarios - Add helpful messages to all assertions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d351260f-2bbe-4728-ba3a-d5166fa126bd Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves the quality and coverage of scripts/lint_error_messages_test.go by adopting testify assertions and adding missing tests for regex pattern matching and analyzeFile.
Changes:
- Replaced manual
t.Errorfchecks withtestify/assert+testify/requirefor clearer, more consistent test failures. - Expanded
TestPatternMatchingto cover all compiled regexes with both match and non-match cases. - Added
TestAnalyzeFileto exercise AST-walking behavior across compliant, non-compliant, mixed, empty, and unparseable files; removed now-unused indirect deps via tidy.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/lint_error_messages_test.go | Switch to testify, expand regex coverage, and add new analyzeFile tests using temp Go files. |
| go.mod | Removes an unused indirect dependency (github.com/sahilm/fuzzy). |
| go.sum | Drops stale checksums consistent with dependency cleanup/tidy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case "isWrappedError": | ||
| result = isWrappedError.MatchString(tt.message) | ||
| case "hasDocLink": | ||
| result = hasDocLink.MatchString(tt.message) |
There was a problem hiding this comment.
In TestPatternMatching, the switch tt.pattern has no default case. If a new/typo pattern string is added with want: false, the test will silently pass because result stays at the zero value (false). Consider adding a default branch that fails the test (e.g., via t.Fatalf) when an unknown pattern key is encountered, so incorrect test cases don’t get masked.
| result = hasDocLink.MatchString(tt.message) | |
| result = hasDocLink.MatchString(tt.message) | |
| default: | |
| t.Fatalf("unknown pattern %q in test %q", tt.pattern, tt.name) |
scripts/lint_error_messages_test.gohad zero testify usage and incomplete coverage — baret.Errorfpatterns, an unusedwantContainfield, only 3 of 7 regex patterns tested, and no tests foranalyzeFile.Changes
if x != y { t.Errorf(...) }patterns withassert.Equal,assert.Nil/assert.NotNil,assert.NotEmpty,assert.Contains; userequire.NoErrorfor setup steps inTestAnalyzeFileTestCheckErrorQuality: Simplified pass/fail branches to singleassert.Nil/assert.NotNilwith descriptive messagesTestSuggestImprovement:wantContainis now actually asserted viaassert.Containsinstead of being ignoredTestPatternMatching: Expanded from 4 cases (3 patterns) to 22 cases covering all 7 compiled regex patterns —hasExample,hasExpected,isValidationError,isFormatError,isTypeError,isEnumError,isWrappedError,hasDocLink— each with both a match and a non-match caseTestAnalyzeFile(new): 5 subtests exercising the core AST-walking function across compliant, non-compliant, mixed, empty, and unparseable files