Skip to content

Conversation

@osterman
Copy link
Member

what

  • Add new lintroller rule testkit-required to enforce TestKit usage in cmd package tests
  • Rule detects tests that modify RootCmd state without calling NewTestKit
  • Only flags state modifications (Execute, SetArgs, ParseFlags, flag modifications)
  • Does not flag read-only access to RootCmd (Commands, Find, etc.)

why

  • RootCmd is global state shared across all tests in the cmd package
  • Without TestKit cleanup, flag values and state leak between tests causing mysterious failures
  • Tests pass in isolation but fail when run together due to state pollution
  • Enforcing TestKit usage prevents hard-to-debug test failures

references

  • Implements pattern described in CLAUDE.md Test Isolation section
  • TestKit pattern follows Go 1.15+ testing.TB interface idiom
  • Similar to existing rules: os.Setenv → t.Setenv, os.MkdirTemp → t.TempDir

details

Rule Behavior

Requires TestKit:

  • RootCmd.Execute() or RootCmd.ExecuteC()
  • RootCmd.SetArgs()
  • RootCmd.ParseFlags()
  • RootCmd.PersistentFlags().Set() or RootCmd.Flags().Set()

Does NOT require TestKit:

  • RootCmd.Commands() - read-only
  • RootCmd.Find() - read-only
  • Any other read-only access

Error Message

test function TestFoo modifies RootCmd state but does not call NewTestKit;
use _ = NewTestKit(t) to ensure proper RootCmd state cleanup
(only needed for Execute/SetArgs/ParseFlags/flag modifications, not read-only access)

Example Fix

// Before (fails linter)
func TestMyCommand(t *testing.T) {
    RootCmd.SetArgs([]string{"terraform", "plan"})
    err := RootCmd.Execute()
    // ...
}

// After (passes linter)
func TestMyCommand(t *testing.T) {
    _ = NewTestKit(t)  // Automatic cleanup
    RootCmd.SetArgs([]string{"terraform", "plan"})
    err := RootCmd.Execute()
    // ...
}

Implementation

  • New rule: tools/lintroller/rule_testkit.go
  • Test coverage: tools/lintroller/testdata/src/testkit/bad_test.go
  • Configuration: .golangci.yml enables testkit-required: true
  • Fixed existing tests: cmd/root_test.go (2 tests needed TestKit, 1 was read-only)

Testing

All lintroller tests pass:

go test ./tools/lintroller/...
# PASS

Rule successfully catches violations during pre-commit.

osterman and others added 2 commits October 23, 2025 14:28
- Add TestKitRule to detect cmd package tests using RootCmd without NewTestKit
- Tests using RootCmd, RootCmd.Execute(), or RootCmd.SetArgs() must call NewTestKit for proper cleanup
- Add test coverage for the new rule with bad_test.go examples
- Update .golangci.yml to enable testkit-required rule
- Update plugin settings and documentation
- Only flag Execute/SetArgs when called on RootCmd (not other cobra commands)
- Fix root_test.go to use NewTestKit in tests accessing RootCmd

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Changed rule to only flag tests that MODIFY RootCmd state (Execute, SetArgs, ParseFlags, flag modifications)
- Read-only access to RootCmd (Commands(), Find(), etc.) no longer requires TestKit
- Updated error message to clarify when TestKit is needed
- Updated testdata to demonstrate read-only vs modification patterns
- Removed unnecessary TestKit call from TestInitFunction (read-only test)
- TestVersionFlagParsing and TestVersionFlagExecutionPath still need TestKit (they modify flags)

This fixes false positives where tests only read RootCmd state and don't need cleanup.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@osterman osterman requested a review from a team as a code owner October 23, 2025 19:41
@github-actions github-actions bot added the size/m Medium size PR label Oct 23, 2025
@github-actions
Copy link

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@osterman osterman added the no-release Do not create a new release (wait for additional code changes) label Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release Do not create a new release (wait for additional code changes) size/m Medium size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants