From ecaa5e101bf0b714331fd08406c409c8e4cc1c8d Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 20 Jun 2025 08:18:38 -0700 Subject: [PATCH] Fix all usetesting linter errors - Replace os.MkdirTemp() with t.TempDir() in test files - Replace os.Setenv() with t.Setenv() in test files - Remove manual environment cleanup code (t.Setenv automatically restores) - Remove unused "os" imports from files that no longer use os package --- .claude/settings.local.json | 4 ++- TODO.md | 5 +++ internal/cli/cli_test.go | 12 +------ internal/cli/generate.go | 3 +- internal/cli/integration_test.go | 8 ++--- internal/secret/debug_test.go | 21 +++--------- internal/secret/passphrase_test.go | 20 ++--------- internal/secret/pgpunlock_test.go | 55 ++++-------------------------- internal/secret/secret_test.go | 16 ++------- internal/vault/integration_test.go | 29 ++-------------- internal/vault/vault_test.go | 24 ++----------- 11 files changed, 34 insertions(+), 163 deletions(-) diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 22a9190..c317f09 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -14,7 +14,9 @@ "Bash(make:*)", "Bash(golangci-lint run:*)", "Bash(git add:*)", - "Bash(gofumpt:*)" + "Bash(gofumpt:*)", + "Bash(git commit:*)", + "Bash(git push:*)" ], "deny": [] } diff --git a/TODO.md b/TODO.md index ea81fd1..c53d963 100644 --- a/TODO.md +++ b/TODO.md @@ -6,6 +6,11 @@ prioritized from most critical (top) to least critical (bottom). ## Code Cleanups +* none of the integration tests should be searching for a binary or trying + to execute another process. the integration tests cannot make another + process or depend on a compiled file, they must do all of their testing in + the current (test) process. + * we shouldn't be passing around a statedir, it should be read from the environment or default. diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 239585b..4d95adc 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -37,19 +37,9 @@ func TestCLIInstanceWithFs(t *testing.T) { func TestDetermineStateDir(t *testing.T) { // Test the determineStateDir function from the secret package - // Save original environment and restore it after test - originalStateDir := os.Getenv(secret.EnvStateDir) - defer func() { - if originalStateDir == "" { - os.Unsetenv(secret.EnvStateDir) - } else { - os.Setenv(secret.EnvStateDir, originalStateDir) - } - }() - // Test with environment variable set testEnvDir := "/test-env-dir" - os.Setenv(secret.EnvStateDir, testEnvDir) + t.Setenv(secret.EnvStateDir, testEnvDir) stateDir := secret.DetermineStateDir("") if stateDir != testEnvDir { diff --git a/internal/cli/generate.go b/internal/cli/generate.go index 4f08b2c..422ad33 100644 --- a/internal/cli/generate.go +++ b/internal/cli/generate.go @@ -28,7 +28,8 @@ func newGenerateMnemonicCmd() *cobra.Command { return &cobra.Command{ Use: "mnemonic", Short: "Generate a random BIP39 mnemonic phrase", - Long: `Generate a cryptographically secure random BIP39 mnemonic phrase that can be used with 'secret init' or 'secret import'.`, + Long: `Generate a cryptographically secure random BIP39 mnemonic phrase that can be used with ` + + `'secret init' or 'secret import'.`, RunE: func(cmd *cobra.Command, args []string) error { cli := NewCLIInstance() return cli.GenerateMnemonic(cmd) diff --git a/internal/cli/integration_test.go b/internal/cli/integration_test.go index c1a4144..8abe250 100644 --- a/internal/cli/integration_test.go +++ b/internal/cli/integration_test.go @@ -52,8 +52,7 @@ func TestMain(m *testing.M) { // This test serves as both validation and documentation of the program's behavior. func TestSecretManagerIntegration(t *testing.T) { // Enable debug logging to diagnose issues - os.Setenv("GODEBUG", "berlin.sneak.pkg.secret") - defer os.Unsetenv("GODEBUG") + t.Setenv("GODEBUG", "berlin.sneak.pkg.secret") // Reinitialize debug logging to pick up the environment variable change secret.InitDebugLogging() @@ -66,8 +65,7 @@ func TestSecretManagerIntegration(t *testing.T) { tempDir := t.TempDir() // Set environment variables for the test - os.Setenv("SB_SECRET_STATE_DIR", tempDir) - defer os.Unsetenv("SB_SECRET_STATE_DIR") + t.Setenv("SB_SECRET_STATE_DIR", tempDir) // Find the secret binary path (needed for tests that still use exec.Command) wd, err := os.Getwd() @@ -1570,7 +1568,7 @@ func test23ErrorHandling(t *testing.T, tempDir, secretPath, testMnemonic string, cmdOutput, err = cmd.CombinedOutput() assert.Error(t, err, "get without mnemonic should fail") assert.Contains(t, string(cmdOutput), "failed to unlock", "should indicate unlock failure") - os.Setenv("SB_SECRET_MNEMONIC", unsetMnemonic) + t.Setenv("SB_SECRET_MNEMONIC", unsetMnemonic) // Invalid secret names (already tested in test 12) diff --git a/internal/secret/debug_test.go b/internal/secret/debug_test.go index 35180f5..ad0b31d 100644 --- a/internal/secret/debug_test.go +++ b/internal/secret/debug_test.go @@ -3,7 +3,6 @@ package secret import ( "bytes" "log/slog" - "os" "strings" "syscall" "testing" @@ -12,14 +11,8 @@ import ( ) func TestDebugLogging(t *testing.T) { - // Save original GODEBUG and restore it - originalGodebug := os.Getenv("GODEBUG") + // Original GODEBUG will be restored automatically by t.Setenv defer func() { - if originalGodebug == "" { - os.Unsetenv("GODEBUG") - } else { - os.Setenv("GODEBUG", originalGodebug) - } // Re-initialize debug system with original setting InitDebugLogging() }() @@ -55,9 +48,9 @@ func TestDebugLogging(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Set GODEBUG if tt.godebug == "" { - os.Unsetenv("GODEBUG") + t.Setenv("GODEBUG", "") } else { - os.Setenv("GODEBUG", tt.godebug) + t.Setenv("GODEBUG", tt.godebug) } // Re-initialize debug system @@ -104,14 +97,8 @@ func TestDebugLogging(t *testing.T) { func TestDebugFunctions(t *testing.T) { // Enable debug for testing - originalGodebug := os.Getenv("GODEBUG") - os.Setenv("GODEBUG", "berlin.sneak.pkg.secret") + t.Setenv("GODEBUG", "berlin.sneak.pkg.secret") defer func() { - if originalGodebug == "" { - os.Unsetenv("GODEBUG") - } else { - os.Setenv("GODEBUG", originalGodebug) - } InitDebugLogging() }() diff --git a/internal/secret/passphrase_test.go b/internal/secret/passphrase_test.go index bf2b82f..d829a3d 100644 --- a/internal/secret/passphrase_test.go +++ b/internal/secret/passphrase_test.go @@ -19,11 +19,7 @@ func TestPassphraseUnlockerWithRealFS(t *testing.T) { } // Create a temporary directory for our tests - tempDir, err := os.MkdirTemp("", "secret-passphrase-test-") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) // Clean up after test + tempDir := t.TempDir() // Use the real filesystem fs := afero.NewOsFs() @@ -131,18 +127,8 @@ func TestPassphraseUnlockerWithRealFS(t *testing.T) { } }) - // Save original environment variables and set test ones - oldPassphrase := os.Getenv(secret.EnvUnlockPassphrase) - os.Setenv(secret.EnvUnlockPassphrase, testPassphrase) - - // Clean up after test - defer func() { - if oldPassphrase != "" { - os.Setenv(secret.EnvUnlockPassphrase, oldPassphrase) - } else { - os.Unsetenv(secret.EnvUnlockPassphrase) - } - }() + // Set test environment variables + t.Setenv(secret.EnvUnlockPassphrase, testPassphrase) // Test getting identity from environment variable t.Run("GetIdentityFromEnv", func(t *testing.T) { diff --git a/internal/secret/pgpunlock_test.go b/internal/secret/pgpunlock_test.go index e8a0c22..ea4cc2f 100644 --- a/internal/secret/pgpunlock_test.go +++ b/internal/secret/pgpunlock_test.go @@ -131,11 +131,7 @@ func TestPGPUnlockerWithRealFS(t *testing.T) { } // Create a temporary directory for our tests - tempDir, err := os.MkdirTemp("", "secret-pgp-test-") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) // Clean up after test + tempDir := t.TempDir() // Create a temporary GNUPGHOME gnupgHomeDir := filepath.Join(tempDir, "gnupg") @@ -143,20 +139,8 @@ func TestPGPUnlockerWithRealFS(t *testing.T) { t.Fatalf("Failed to create GNUPGHOME: %v", err) } - // Save original GNUPGHOME - origGnupgHome := os.Getenv("GNUPGHOME") - // Set new GNUPGHOME - os.Setenv("GNUPGHOME", gnupgHomeDir) - - // Clean up environment after test - defer func() { - if origGnupgHome != "" { - os.Setenv("GNUPGHOME", origGnupgHome) - } else { - os.Unsetenv("GNUPGHOME") - } - }() + t.Setenv("GNUPGHOME", gnupgHomeDir) // Test passphrase for GPG key testPassphrase := "test123" @@ -182,7 +166,7 @@ Passphrase: ` + testPassphrase + ` // Generate GPG key with batch mode t.Log("Generating GPG key...") - _, err = runGPGWithPassphrase(gnupgHomeDir, testPassphrase, + _, err := runGPGWithPassphrase(gnupgHomeDir, testPassphrase, []string{"--gen-key", batchFile}, nil) if err != nil { t.Fatalf("Failed to generate GPG key: %v", err) @@ -224,15 +208,7 @@ Passphrase: ` + testPassphrase + ` t.Logf("Generated GPG fingerprint: %s", fingerprint) // Set the GPG_AGENT_INFO to empty to ensure gpg-agent doesn't interfere - oldAgentInfo := os.Getenv("GPG_AGENT_INFO") - os.Setenv("GPG_AGENT_INFO", "") - defer func() { - if oldAgentInfo != "" { - os.Setenv("GPG_AGENT_INFO", oldAgentInfo) - } else { - os.Unsetenv("GPG_AGENT_INFO") - } - }() + t.Setenv("GPG_AGENT_INFO", "") // Use the real filesystem fs := afero.NewOsFs() @@ -240,28 +216,9 @@ Passphrase: ` + testPassphrase + ` // Test data testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - // Save original environment variable - oldMnemonic := os.Getenv(secret.EnvMnemonic) - oldGPGKeyID := os.Getenv(secret.EnvGPGKeyID) - // Set test environment variables - os.Setenv(secret.EnvMnemonic, testMnemonic) - os.Setenv(secret.EnvGPGKeyID, keyID) - - // Clean up after test - defer func() { - if oldMnemonic != "" { - os.Setenv(secret.EnvMnemonic, oldMnemonic) - } else { - os.Unsetenv(secret.EnvMnemonic) - } - - if oldGPGKeyID != "" { - os.Setenv(secret.EnvGPGKeyID, oldGPGKeyID) - } else { - os.Unsetenv(secret.EnvGPGKeyID) - } - }() + t.Setenv(secret.EnvMnemonic, testMnemonic) + t.Setenv(secret.EnvGPGKeyID, keyID) // Set up vault structure for testing stateDir := tempDir diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go index b874d0c..df9746d 100644 --- a/internal/secret/secret_test.go +++ b/internal/secret/secret_test.go @@ -128,19 +128,9 @@ func TestPerSecretKeyFunctionality(t *testing.T) { // Create an in-memory filesystem for testing fs := afero.NewMemMapFs() - // Set up test environment variables - oldMnemonic := os.Getenv(EnvMnemonic) - defer func() { - if oldMnemonic == "" { - os.Unsetenv(EnvMnemonic) - } else { - os.Setenv(EnvMnemonic, oldMnemonic) - } - }() - // Set test mnemonic for direct encryption/decryption testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - os.Setenv(EnvMnemonic, testMnemonic) + t.Setenv(EnvMnemonic, testMnemonic) // Set up a test vault structure baseDir := "/test-config/berlin.sneak.pkg.secret" @@ -312,9 +302,7 @@ func TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex(t *testing.T) { // Set up test mnemonic testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - originalEnv := os.Getenv(EnvMnemonic) - os.Setenv(EnvMnemonic, testMnemonic) - defer os.Setenv(EnvMnemonic, originalEnv) + t.Setenv(EnvMnemonic, testMnemonic) // Create temporary directory for vaults fs := afero.NewOsFs() diff --git a/internal/vault/integration_test.go b/internal/vault/integration_test.go index d33a415..393a88b 100644 --- a/internal/vault/integration_test.go +++ b/internal/vault/integration_test.go @@ -13,11 +13,7 @@ import ( func TestVaultWithRealFilesystem(t *testing.T) { // Create a temporary directory for our tests - tempDir, err := os.MkdirTemp("", "secret-test-") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) // Clean up after test + tempDir := t.TempDir() // Use the real filesystem fs := afero.NewOsFs() @@ -25,28 +21,9 @@ func TestVaultWithRealFilesystem(t *testing.T) { // Test mnemonic testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - // Save original environment variables - oldMnemonic := os.Getenv(secret.EnvMnemonic) - oldPassphrase := os.Getenv(secret.EnvUnlockPassphrase) - // Set test environment variables - os.Setenv(secret.EnvMnemonic, testMnemonic) - os.Setenv(secret.EnvUnlockPassphrase, "test-passphrase") - - // Clean up after test - defer func() { - if oldMnemonic != "" { - os.Setenv(secret.EnvMnemonic, oldMnemonic) - } else { - os.Unsetenv(secret.EnvMnemonic) - } - - if oldPassphrase != "" { - os.Setenv(secret.EnvUnlockPassphrase, oldPassphrase) - } else { - os.Unsetenv(secret.EnvUnlockPassphrase) - } - }() + t.Setenv(secret.EnvMnemonic, testMnemonic) + t.Setenv(secret.EnvUnlockPassphrase, "test-passphrase") // Test symlink handling t.Run("SymlinkHandling", func(t *testing.T) { diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 0724942..7fc27d4 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -1,7 +1,6 @@ package vault import ( - "os" "path/filepath" "testing" @@ -11,29 +10,10 @@ import ( ) func TestVaultOperations(t *testing.T) { - // Save original environment variables - oldMnemonic := os.Getenv(secret.EnvMnemonic) - oldPassphrase := os.Getenv(secret.EnvUnlockPassphrase) - - // Clean up after test - defer func() { - if oldMnemonic != "" { - os.Setenv(secret.EnvMnemonic, oldMnemonic) - } else { - os.Unsetenv(secret.EnvMnemonic) - } - - if oldPassphrase != "" { - os.Setenv(secret.EnvUnlockPassphrase, oldPassphrase) - } else { - os.Unsetenv(secret.EnvUnlockPassphrase) - } - }() - // Set test environment variables testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - os.Setenv(secret.EnvMnemonic, testMnemonic) - os.Setenv(secret.EnvUnlockPassphrase, "test-passphrase") + t.Setenv(secret.EnvMnemonic, testMnemonic) + t.Setenv(secret.EnvUnlockPassphrase, "test-passphrase") // Use in-memory filesystem fs := afero.NewMemMapFs()