From 666e2438b0b3c7e6c848e9a2a619dac7cb30d77b Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 5 Oct 2026 23:50:49 +0000 Subject: [PATCH] Make the tests fast under the race detector (closes #120) Deriving keys from passphrases with scrypt, slow on purpose, took most of the test time under -race. secret.ScryptWorkFactor, when not zero, replaces age's work factor when a passphrase encrypts; the tests of internal/secret, internal/vault and internal/cli set it to 1 in TestMain, and the program never sets it. TestRemovalAsksWithoutHoldingLock and TestFailedCommandReleasesLock time the in-memory lock all tests share, so they no longer run in parallel. TestConcurrentAddsKeepEveryVersion and TestGetCommandOutputsToStdout time nothing and now do. The script/cibuild comment no longer says tests are skipped without its memlock ulimit. Model: opus-5-5 --- TODO.md | 14 ++++++++++++++ internal/cli/confirm_test.go | 4 ++-- internal/cli/integration_test.go | 6 +++++- internal/cli/lock_test.go | 10 ++++++---- internal/cli/stdout_stderr_test.go | 8 ++++---- internal/secret/crypto.go | 15 +++++++++++++++ internal/secret/secret_test.go | 8 ++++++++ internal/vault/vault_test.go | 9 +++++++++ script/cibuild | 6 +++--- 9 files changed, 66 insertions(+), 14 deletions(-) diff --git a/TODO.md b/TODO.md index 02f2cf9..a942f02 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,20 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-06: The tests run quickly with the race detector on + (https://git.eeqj.de/sneak/secret/issues/120). Most of their time went to + deriving keys from passphrases with scrypt, which is slow on purpose. The new + `secret.ScryptWorkFactor`, when not zero, replaces age's scrypt work factor + when a passphrase encrypts; the tests of `internal/secret`, `internal/vault` + and `internal/cli` set it to 1 before any test runs, and the program never + sets it. `TestRemovalAsksWithoutHoldingLock` and + `TestFailedCommandReleasesLock` no longer run in parallel with other tests: + each waits at most 10 seconds for the in-memory lock that every test in the + package shares, and other tests' commands held it longer. + `TestConcurrentAddsKeepEveryVersion`, which times nothing, and + `TestGetCommandOutputsToStdout`, which no longer sets an environment variable + its commands do not read, now run in parallel. The `script/cibuild` comment no + longer says that tests are skipped without its memlock ulimit. - 2026-10-05: No test stores a secret larger than 1 MiB (https://git.eeqj.de/sneak/secret/issues/52). The size tests for `secret add`, `secret import` and the stdin buffer no longer try 2 MB, 10 MB, 99 MB, 100 MB diff --git a/internal/cli/confirm_test.go b/internal/cli/confirm_test.go index 32a56b8..af2ec91 100644 --- a/internal/cli/confirm_test.go +++ b/internal/cli/confirm_test.go @@ -353,9 +353,9 @@ func TestRemovalWithoutTerminalFailsAtOnce(t *testing.T) { // for its answer, another command can take the state directory lock and // change the secret, and that the removal then removes nothing, since the // secret is no longer what the question named. +// +//nolint:paralleltest // times commands against the in-memory lock all tests share func TestRemovalAsksWithoutHoldingLock(t *testing.T) { - t.Parallel() - r := newRemoval(t, "rm") answers, answerWriter := io.Pipe() diff --git a/internal/cli/integration_test.go b/internal/cli/integration_test.go index d572e5c..9e66629 100644 --- a/internal/cli/integration_test.go +++ b/internal/cli/integration_test.go @@ -52,8 +52,12 @@ func runSecretWithStdin(stdin string, env map[string]string, args ...string) (st return cli.ExecuteCommandInProcess(args, stdin, env) } -// TestMain runs before all tests and ensures the binary is built +// TestMain runs before all tests and ensures the binary is built. It also +// makes passphrase encryption in the tests cheap (see +// secret.ScryptWorkFactor); the binary keeps age's work factor. func TestMain(m *testing.M) { + secret.ScryptWorkFactor = 1 + // Get the current working directory wd, err := os.Getwd() if err != nil { diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index 502553f..caa2ee3 100644 --- a/internal/cli/lock_test.go +++ b/internal/cli/lock_test.go @@ -94,9 +94,9 @@ func numbered(prefix string, count int) []string { // lock, adds of a new secret all find it absent and replace each other, and // forced adds read the same highest version number and overwrite each // other's version. With it they behave as if run one after another. -// -//nolint:paralleltest // times commands against the in-memory lock all tests share func TestConcurrentAddsKeepEveryVersion(t *testing.T) { + t.Parallel() + mnemonic := testMnemonicBuffer(t) const adds = 8 @@ -110,6 +110,8 @@ func TestConcurrentAddsKeepEveryVersion(t *testing.T) { {"real", afero.NewOsFs(), t.TempDir()}, } { t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := vault.CreateVault(tc.fs, tc.stateDir, "default", mnemonic, nil) require.NoError(t, err) @@ -235,9 +237,9 @@ func TestEncryptPipedIntoAdd(t *testing.T) { // TestFailedCommandReleasesLock checks that a command failing after it // took the state directory lock leaves the lock free for the next command. +// +//nolint:paralleltest // times commands against the in-memory lock all tests share func TestFailedCommandReleasesLock(t *testing.T) { - t.Parallel() - fs := afero.NewMemMapFs() cli := NewCLIInstanceWithStateDir(fs, testStateDir) diff --git a/internal/cli/stdout_stderr_test.go b/internal/cli/stdout_stderr_test.go index a0916ce..1b2b0c0 100644 --- a/internal/cli/stdout_stderr_test.go +++ b/internal/cli/stdout_stderr_test.go @@ -15,11 +15,11 @@ import ( // TestGetCommandOutputsToStdout tests that 'secret get' outputs the secret // value to stdout, not stderr func TestGetCommandOutputsToStdout(t *testing.T) { - // Create a temporary directory for our vault - tempDir := t.TempDir() + t.Parallel() - // Set environment variables for the test - t.Setenv(secret.EnvStateDir, tempDir) + // Create a temporary directory for our vault; each command is given it + // in its environment + tempDir := t.TempDir() // Find the secret binary path wd, err := filepath.Abs("../..") diff --git a/internal/secret/crypto.go b/internal/secret/crypto.go index 9c017a1..f6005f0 100644 --- a/internal/secret/crypto.go +++ b/internal/secret/crypto.go @@ -28,6 +28,17 @@ var ( // terminal to read the mnemonic from, reading it failed, or it was empty. var ErrMnemonicNotRead = errors.New("failed to read mnemonic") +// ScryptWorkFactor is, when not zero, the scrypt work factor that +// EncryptWithPassphrase uses instead of age's, 18: log2 of scrypt's cost +// parameter N. Deriving a key with age's takes about a second and 256 MiB, on +// purpose, since so does every guess at the passphrase. Only tests set it, +// lower, before any test runs, so that the passphrase unlockers they create +// cost nothing; the program leaves it zero. Decryption takes the work factor +// from the encrypted data, so it needs no setting. +// +//nolint:gochecknoglobals // set by the tests of the packages that use this one +var ScryptWorkFactor int + // EncryptToRecipient encrypts data to a recipient using age // The data parameter should be a LockedBuffer for secure memory handling func EncryptToRecipient( @@ -142,6 +153,10 @@ func EncryptWithPassphrase( return nil, fmt.Errorf("failed to create scrypt recipient: %w", err) } + if ScryptWorkFactor != 0 { + recipient.SetWorkFactor(ScryptWorkFactor) + } + return EncryptToRecipient(data, recipient) } diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go index e5dae85..94bd5d4 100644 --- a/internal/secret/secret_test.go +++ b/internal/secret/secret_test.go @@ -25,6 +25,14 @@ var ( errNotImplementedInMock = errors.New("not implemented in mock") ) +// TestMain makes passphrase encryption in the tests cheap; see +// ScryptWorkFactor. +func TestMain(m *testing.M) { + ScryptWorkFactor = 1 + + os.Exit(m.Run()) +} + // MockVault is a test implementation of the VaultInterface type MockVault struct { name string diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 2b6967d..72e0243 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -3,6 +3,7 @@ package vault_test import ( "bytes" "errors" + "os" "path/filepath" "slices" "testing" @@ -28,6 +29,14 @@ const ( testPassphrase = "test-passphrase" ) +// TestMain makes passphrase encryption in the tests cheap; see +// secret.ScryptWorkFactor. +func TestMain(m *testing.M) { + secret.ScryptWorkFactor = 1 + + os.Exit(m.Run()) +} + // testMnemonicBuffer returns testMnemonic in a locked buffer that is // destroyed when the test ends. func testMnemonicBuffer(t *testing.T) *memguard.LockedBuffer { diff --git a/script/cibuild b/script/cibuild index 58bf15f..bb21515 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,9 +1,9 @@ #!/bin/sh # script/cibuild: run the CI build. The Dockerfile runs script/check # (via make check), so a successful build implies all checks pass. -# The Gitea workflow runs this on push. The memlock ulimit lets the tests -# that lock large secrets in memory (memguard mlocks them) run; under the -# lower limit of a plain `docker build .` they are skipped. +# The Gitea workflow runs this on push. The memlock ulimit lifts the limit +# on memory the tests lock (memguard mlocks secrets); they also pass under +# the lower limit of a plain `docker build .`. # A cached build checks nothing: a new CHECK_EPOCH on every run makes the # Dockerfile's check steps run again on an unchanged tree, while its base # images and module downloads stay cached.