TDD Quality Assessment for PR #47: Config Management Commands

Overall Assessment: Good with Minor Gaps

The tests follow established codebase patterns well and cover the core functionality. Here's a detailed breakdown:

1. Test Comprehensiveness

show_test.go - Tests helper functions well:

• maskToken: Normal, short, exactly 8 chars, 9 chars, empty token
• getValueAndSource: Env precedence, file fallback, not set
• formatValueWithSource: Value with source, empty value

test_test.go - Good API mocking:

• Success case (200 OK)
• Auth failure (401)
• Server error (500)

clear_test.go - Good confirmation flow coverage:

• File not found
• Force flag (skips prompt)
• Confirmation accepted ("y\n")
• Confirmation cancelled ("n\n")

2. Edge Cases Covered

3. Pattern Consistency Analysis

Strengths:

• Uses httptest.NewServer() for API mocking (matches init_test.go, delete_test.go)
• Uses t.TempDir() for file operations
• Uses t.Setenv() for environment variable isolation
• Uses table-driven tests for maskToken and getValueAndSource
• Uses strings.NewReader() for injectable stdin

Inconsistencies:

1. clear.go uses custom stdin injection (clearOptions.stdin) instead of using opts.Stdin from root.Options like page/delete.go does
2. clear.go uses manual confirmation parsing with fmt.Fscanln() instead of the shared prompt.Confirm() helper used in page/delete.go
3. clear_test.go doesn't test the "yes" confirmation variant - the code accepts both "y" and "yes", but only "y\n" is tested
4. test_test.go doesn't test the no-client case - when opts.APIClient() returns an error (missing config)

4. What's Missing

High Priority:

1. Confirmation edge cases for clear command:

   • "yes\n" - accepted but not tested
   • "\n" - empty input behavior
   • " y \n" - whitespace handling (code uses strings.TrimSpace)
   • "Y\n" - uppercase (code converts to lowercase, should work)

2. No test for runShow() function - only helper functions are tested. Should test:

   • Output when config file exists
   • Output when config file doesn't exist
   • Output with env vars set
   • Token masking in actual output

3. test.go: No test for configuration error path - when opts.APIClient() fails

Medium Priority:
4. clear.go: No test for os.Remove() failure - e.g., permission denied
5. clear.go: No test for environment variable warning output - the code checks for active env vars after deletion

Low Priority:
6. show.go: getEnvVarName() not tested - it determines which env var name to display

5. Recommended Additions

// clear_test.go - Add these test cases to existing tests

func TestRunClear_ConfirmationYes(t *testing.T) {
    // Test "yes\n" (full word) works
}

func TestRunClear_ConfirmationUppercase(t *testing.T) {
    // Test "Y\n" works
}

func TestRunClear_EmptyInput(t *testing.T) {
    // Test "\n" cancels
}

func TestRunClear_PermissionDenied(t *testing.T) {
    // Create read-only file, verify error handling
}

// show_test.go - Add runShow() tests

func TestRunShow_WithConfigFile(t *testing.T) {
    // Create temp config, verify output format
}

func TestRunShow_WithEnvVars(t *testing.T) {
    // Set env vars, verify source attribution
}

// test_test.go - Add config error test

func TestRunTest_NoConfig(t *testing.T) {
    // Don't set test client, verify "configuration error" message
}

6. Code Style Note

The clear.go implementation should consider using the shared prompt.Confirm() helper from github.com/open-cli-collective/atlassian-go/prompt (used in page/delete.go) for consistency. This would also ensure the confirmation behavior is tested via that shared helper rather than needing duplicate tests.

Summary

The PR demonstrates good TDD practices with:

• Unit tests for all helper functions
• API mocking with httptest
• t.TempDir() for file isolation
• Table-driven tests where appropriate

Main gaps are:

1. No integration test for runShow()
2. Missing confirmation edge cases for clear
3. Slight inconsistency in using custom stdin vs shared prompt.Confirm()

Recommendation: Merge-ready with optional follow-up for the missing runShow() tests and confirmation edge cases.