mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
c650aabbef
My agent added `//nolint:testpackage` to a test file on one of my PRs. Again. This PR cleans it up across the entire repo and updates the in-repo conventions so future agents stop doing it. The repo already has a precedent for white-box tests that need to touch unexported symbols: `*_internal_test.go` (145+ existing files). The `testpackage` linter's default `skip-regexp` exempts that filename suffix, so the `//nolint:testpackage` directive is unnecessary in every case where someone reached for it. This PR renames 51 such files to `*_internal_test.go` via `git mv` so blame and history follow, and strips the dead directive from 2 files that were already correctly named (`coderd/oauth2provider/authorize_internal_test.go`, `coderd/x/chatd/advisor_internal_test.go`). `.claude/docs/TESTING.md` now documents the rule explicitly under *Test Package Naming*, which is imported into the root `AGENTS.md` via `@.claude/docs/TESTING.md`. The rule: prefer `package foo_test`; if you need internal access, rename the file to `*_internal_test.go` rather than adding a nolint directive.
6.7 KiB
6.7 KiB
Testing Patterns and Best Practices
Testing Best Practices
Avoiding Race Conditions
-
Unique Test Identifiers:
- Never use hardcoded names in concurrent tests
- Use
time.Now().UnixNano()or similar for unique identifiers - Example:
fmt.Sprintf("test-client-%s-%d", t.Name(), time.Now().UnixNano())
-
Database Constraint Awareness:
- Understand unique constraints that can cause test conflicts
- Generate unique values for all constrained fields
- Test name isolation prevents cross-test interference
Testing Patterns
- Use table-driven tests for comprehensive coverage
- Mock external dependencies
- Test both positive and negative cases
- Use
testutil.WaitLongfor timeouts in tests
Timing Issues
NEVER use time.Sleep to mitigate timing issues. If an issue seems like
it should use time.Sleep, read through https://github.com/coder/quartz
and specifically the README to better understand how to handle timing
issues.
Test Package Naming
- Black-box tests: Default to a
package foo_testtest file (e.g.,identityprovider_test). This is what thetestpackagelinter enforces. - White-box / internal tests: When a test needs to touch unexported
symbols, put it in a file named
*_internal_test.gowithpackage foo. Thetestpackagelinter'sskip-regexpalready exempts that filename suffix, so no//nolint:testpackagedirective is needed. - Do not add
//nolint:testpackage. If a test needs internal access, rename the file to*_internal_test.goinstead. A directive plus a justification comment is strictly worse than the established naming convention, and the repo standardizes on the latter.
RFC Protocol Testing
Compliance Test Coverage
- Test all RFC-defined error codes and responses
- Validate proper HTTP status codes for different scenarios
- Test protocol-specific edge cases (URI formats, token formats, etc.)
Security Boundary Testing
- Test client isolation and privilege separation
- Verify information disclosure protections
- Test token security and proper invalidation
Test Organization
Test File Structure
coderd/
├── oauth2.go # Implementation
├── oauth2_test.go # Main tests
├── oauth2_test_helpers.go # Test utilities
└── oauth2_validation.go # Validation logic
Test Categories
- Unit Tests: Test individual functions in isolation
- Integration Tests: Test API endpoints with database
- End-to-End Tests: Full workflow testing
- Race Tests: Concurrent access testing
Test Commands
Running Tests
| Command | Purpose |
|---|---|
make test |
Run all Go tests |
make test RUN=TestFunctionName |
Run specific test |
go test -v ./path/to/package -run TestFunctionName |
Run test with verbose output |
make test-race |
Run tests with Go race detector |
make test-e2e |
Run end-to-end tests |
Frontend Testing
| Command | Purpose |
|---|---|
pnpm test |
Run frontend tests |
pnpm check |
Run code checks |
Common Testing Issues
Database-Related
- SQL type errors - Use
sql.Null*types for nullable fields - Race conditions in tests - Use unique identifiers instead of hardcoded names
OAuth2 Testing
- PKCE tests failing - Verify both authorization code storage and token exchange handle PKCE fields
- Resource indicator validation failing - Ensure database stores and retrieves resource parameters correctly
OAuth2 Test Scripts
- Full suite:
./scripts/oauth2/test-mcp-oauth2.sh - Manual testing:
./scripts/oauth2/test-manual-flow.sh
General Issues
- Missing newlines - Ensure files end with newline character
- Package naming errors - Use
package_testnaming for test files - Log message formatting errors - Use lowercase, descriptive messages without special characters
Systematic Testing Approach
Multi-Issue Problem Solving
When facing multiple failing tests or complex integration issues:
-
Identify Root Causes:
- Run failing tests individually to isolate issues
- Use LSP tools to trace through call chains
- Check both compilation and runtime errors
-
Fix in Logical Order:
- Address compilation issues first (imports, syntax)
- Fix authorization and RBAC issues next
- Resolve business logic and validation issues
- Handle edge cases and race conditions last
-
Verification Strategy:
- Test each fix individually before moving to next issue
- Use
make lintandmake genafter database changes - Verify RFC compliance with actual specifications
- Run comprehensive test suites before considering complete
Test Data Management
Unique Test Data
// Good: Unique identifiers prevent conflicts
clientName := fmt.Sprintf("test-client-%s-%d", t.Name(), time.Now().UnixNano())
// Bad: Hardcoded names cause race conditions
clientName := "test-client"
Test Cleanup
func TestSomething(t *testing.T) {
// Setup
client := coderdtest.New(t, nil)
// Test code here
// Cleanup happens automatically via t.Cleanup() in coderdtest
}
Test Utilities
Common Test Patterns
// Table-driven tests
tests := []struct {
name string
input InputType
expected OutputType
wantErr bool
}{
{
name: "valid input",
input: validInput,
expected: expectedOutput,
wantErr: false,
},
// ... more test cases
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result, err := functionUnderTest(tt.input)
if tt.wantErr {
require.Error(t, err)
return
}
require.NoError(t, err)
require.Equal(t, tt.expected, result)
})
}
Test Assertions
// Use testify/require for assertions
require.NoError(t, err)
require.Equal(t, expected, actual)
require.NotNil(t, result)
require.True(t, condition)
Performance Testing
Load Testing
- Use
scaletest/directory for load testing scenarios - Run
./scaletest/scaletest.shfor performance testing
Benchmarking
func BenchmarkFunction(b *testing.B) {
for i := 0; i < b.N; i++ {
// Function call to benchmark
_ = functionUnderTest(input)
}
}
Run benchmarks with:
go test -bench=. -benchmem ./package/path