From ea35293a1fdbeee99718d8f6a7a6be4e7e1cd401 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 18:39:41 +0000 Subject: [PATCH] Hold runMu in internal/cli tests that log outside a CLI run (closes #210) A CLI run under runMu points the process-global logger at its own buffers, so a line another test logged outside runMu could land in that run's output. TestCheckQuietShowsFailedFiles caught one: a "loaded manifest" line in the stderr of a passing check -q run. The test helpers that scan or load a manifest now hold runMu themselves, and so do the remaining direct loads and scans, the recordEntry call and the retrying download. Two dotfile tests use manifestPaths instead of loading the manifest themselves. TestRootVerbosityFlags ran on its own only to avoid those lines, so it now runs in parallel. Judgement call: a downloadFile call that gets its answer on the first request does not log, so those calls are left as they are. Model: opus-5-5 --- internal/cli/entry_test.go | 45 ++++++++++++++++--------------- internal/cli/errmsg_test.go | 10 +++++++ internal/cli/export_test.go | 5 ++++ internal/cli/fetch_test.go | 51 +++++++++++++++++++++++++++--------- internal/cli/freshen_test.go | 9 +++++++ 5 files changed, 87 insertions(+), 33 deletions(-) diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 600e571..2d35d92 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -58,9 +58,10 @@ func runCLI(opts *RunOptions) int { // runCLIWithLevel is runCLI that also returns the log level the run left // set, read while runMu still keeps other runs from changing it. Each run // starts at the default level, as a new process does. Before releasing the -// lock it points the process-global logger at io.Discard: other tests log -// outside the lock (manifest loads, scans), and those lines must not land in -// this run's buffers once it has returned and its test is reading them. +// lock it points the process-global logger at io.Discard: test helpers also +// log under runMu outside any run (manifest loads, scans), and those lines +// must not land in this run's buffers once it has returned and its test is +// reading them. func runCLIWithLevel(opts *RunOptions) (int, log.Level) { runMu.Lock() defer runMu.Unlock() @@ -206,13 +207,13 @@ func TestVersionFlagAndCommandMatch(t *testing.T) { // TestRootVerbosityFlags asserts that -q and -v given before any subcommand // name take effect: in the root itself, and in the fetch and export -// subcommands (issue #64). It does not run in parallel: other tests log -// outside runMu (manifest loads, scans), and a line logged while one of these -// runs is in progress lands in its output. -// -//nolint:paralleltest // see above +// subcommands (issue #64). func TestRootVerbosityFlags(t *testing.T) { + t.Parallel() + t.Run("quiet with no subcommand hides the banner", func(t *testing.T) { + t.Parallel() + loud := testOpts([]string{testApp}, afero.NewMemMapFs()) require.Equal(t, 0, runCLI(loud)) assert.Contains(t, testStdout(t, loud), banner) @@ -224,6 +225,8 @@ func TestRootVerbosityFlags(t *testing.T) { }) t.Run("quiet before fetch hides the banner", func(t *testing.T) { + t.Parallel() + opts := testOpts([]string{testApp, "-q", cmdFetch}, afero.NewMemMapFs()) assert.Equal(t, 1, runCLI(opts)) @@ -232,6 +235,8 @@ func TestRootVerbosityFlags(t *testing.T) { }) t.Run("verbose twice before fetch enables debug logging", func(t *testing.T) { + t.Parallel() + opts := testOpts([]string{testApp, "-v", "-v", cmdFetch}, afero.NewMemMapFs()) assert.Equal(t, 1, runCLI(opts)) @@ -239,6 +244,8 @@ func TestRootVerbosityFlags(t *testing.T) { }) t.Run("quiet before export hides the manifest summary", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() manifest := buildTestManifest(t, map[string][]byte{"a.txt": []byte("a")}) require.NoError(t, afero.WriteFile(fs, testManifest, manifest, 0o644)) @@ -771,13 +778,9 @@ func TestGenerateExcludesDotfilesByDefault(t *testing.T) { assert.True(t, exists) // Verify manifest only has 1 file (the non-dotfile) - manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ - Path: testMF, - Fs: fs, - }) - require.NoError(t, err) - assert.Len(t, manifest.Files(), 1) - assert.Equal(t, "file1.txt", manifest.Files()[0].GetPath()) + paths := manifestPaths(t, fs, testMF) + require.Len(t, paths, 1) + assert.Equal(t, "file1.txt", paths[0]) } func TestGenerateWithIncludeDotfiles(t *testing.T) { @@ -798,12 +801,7 @@ func TestGenerateWithIncludeDotfiles(t *testing.T) { require.Equal(t, 0, exitCode) // Verify manifest has 2 files (including dotfile) - manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ - Path: testMF, - Fs: fs, - }) - require.NoError(t, err) - assert.Len(t, manifest.Files(), 2) + assert.Len(t, manifestPaths(t, fs, testMF), 2) } func TestMultipleInputPaths(t *testing.T) { @@ -1224,6 +1222,11 @@ func TestGenerateFailsWithoutForceWhenOutputExists(t *testing.T) { func manifestPaths(t *testing.T, fs afero.Fs, path string) []string { t.Helper() + // Loading a manifest logs to the process-global logger, so serialize + // with the CLI runs. + runMu.Lock() + defer runMu.Unlock() + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ Path: path, Fs: fs, diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index e3dd9c6..8f8e817 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -54,6 +54,11 @@ func runLocked(fn func() error) error { func unsignedChecker(t *testing.T) *mfer.Checker { t.Helper() + // The scan and the manifest load log to the process-global logger, so + // serialize with the CLI runs. + runMu.Lock() + defer runMu.Unlock() + fs := afero.NewMemMapFs() require.NoError(t, fs.MkdirAll("/d", 0o755)) require.NoError(t, afero.WriteFile(fs, "/d/f.txt", []byte("hi"), 0o644)) @@ -183,6 +188,11 @@ func signedManifest(t *testing.T, files map[string][]byte) []byte { func signedChecker(t *testing.T, manifest []byte) *mfer.Checker { t.Helper() + // Loading a manifest logs to the process-global logger, so serialize + // with the CLI runs. + runMu.Lock() + defer runMu.Unlock() + fs := afero.NewMemMapFs() require.NoError(t, afero.WriteFile(fs, "/index.mf", manifest, 0o644)) diff --git a/internal/cli/export_test.go b/internal/cli/export_test.go index 8bb700c..ac76bc2 100644 --- a/internal/cli/export_test.go +++ b/internal/cli/export_test.go @@ -20,6 +20,11 @@ const testCmdExport = "export" func buildTestManifest(t *testing.T, files map[string][]byte) []byte { t.Helper() + // The scan logs to the process-global logger, so serialize with the + // CLI runs. + runMu.Lock() + defer runMu.Unlock() + sourceFs := afero.NewMemMapFs() for path, content := range files { require.NoError(t, sourceFs.MkdirAll("/", 0o755)) diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index 98b5bdc..fa773bd 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -171,6 +171,11 @@ func TestResolveManifestURL(t *testing.T) { func scanToManifest(t *testing.T, sourceFs afero.Fs) []byte { t.Helper() + // The scan logs to the process-global logger, so serialize with the + // CLI runs. + runMu.Lock() + defer runMu.Unlock() + s := mfer.NewScannerWithOptions(&mfer.ScannerOptions{Fs: sourceFs}) require.NoError(t, s.EnumerateFS(sourceFs, "/", nil)) @@ -314,8 +319,11 @@ func TestFetchFromHTTP(t *testing.T) { // Change to a fresh destination directory for the test destDir := chdirTemp(t) - // Parse the manifest to get file entries + // Parse the manifest to get file entries. Loading a manifest logs to + // the process-global logger. + runMu.Lock() manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + runMu.Unlock() require.NoError(t, err) files := manifest.Files() @@ -365,7 +373,10 @@ func TestFetchHashMismatch(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) + // Loading a manifest logs to the process-global logger. + runMu.Lock() manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + runMu.Unlock() require.NoError(t, err) files := manifest.Files() @@ -411,7 +422,10 @@ func TestFetchSizeMismatch(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) + // Loading a manifest logs to the process-global logger. + runMu.Lock() manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + runMu.Unlock() require.NoError(t, err) files := manifest.Files() @@ -523,7 +537,10 @@ func TestFetchProgress(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) + // Loading a manifest logs to the process-global logger. + runMu.Lock() manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + runMu.Unlock() require.NoError(t, err) files := manifest.Files() @@ -897,9 +914,12 @@ func TestGetStopsWaitingWhenCanceled(t *testing.T) { //nolint:paralleltest // changes the process-global working directory func TestDownloadFileRetriesToSuccess(t *testing.T) { content := []byte("the whole file, every byte of it") + manifestData := manifestOf(t, map[string][]byte{testFileTxt: content}) - manifest, err := mfer.NewManifestFromReader(bytes.NewReader( - manifestOf(t, map[string][]byte{testFileTxt: content}))) + // Loading a manifest logs to the process-global logger. + runMu.Lock() + manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + runMu.Unlock() require.NoError(t, err) var requests atomic.Int32 @@ -922,8 +942,11 @@ func TestDownloadFileRetriesToSuccess(t *testing.T) { chdirTemp(t) - err = downloadFile(context.Background(), testClient(), - server.URL+"/"+testFileTxt, ".", testFileTxt, manifest.Files()[0], nil) + // downloadFile logs each retry to the process-global logger. + err = runLocked(func() error { + return downloadFile(context.Background(), testClient(), + server.URL+"/"+testFileTxt, ".", testFileTxt, manifest.Files()[0], nil) + }) require.NoError(t, err) assert.Equal(t, int32(3), requests.Load()) @@ -1404,14 +1427,18 @@ func TestFetchSetsRecordedMode(t *testing.T) { require.NoError(t, afero.WriteFile(sourceFs, "/"+p, content, modes[p])) } - scanner := mfer.NewScannerWithOptions(&mfer.ScannerOptions{ - Fs: sourceFs, - IncludePermissions: true, - }) - require.NoError(t, scanner.EnumerateFS(sourceFs, "/", nil)) - var manifest bytes.Buffer - require.NoError(t, scanner.ToManifest(context.Background(), &manifest, nil)) + + // The scan logs to the process-global logger. + require.NoError(t, runLocked(func() error { + scanner := mfer.NewScannerWithOptions(&mfer.ScannerOptions{ + Fs: sourceFs, + IncludePermissions: true, + }) + require.NoError(t, scanner.EnumerateFS(sourceFs, "/", nil)) + + return scanner.ToManifest(context.Background(), &manifest, nil) + })) tree := fetchTestHandler(manifest.Bytes(), files) diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index bde7f52..a4ab325 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -70,6 +70,11 @@ func runFreshen(t *testing.T, fs afero.Fs, root, manifestPath string) { func manifestFiles(t *testing.T, fs afero.Fs, path string) []*mfer.MFFilePath { t.Helper() + // Loading a manifest logs to the process-global logger, so serialize + // with the CLI runs. + runMu.Lock() + defer runMu.Unlock() + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ Path: path, Fs: fs, @@ -436,7 +441,11 @@ func TestFreshenRecordEntryMtimePresence(t *testing.T) { s := &freshenScanner{ existingByPath: map[string]*mfer.MFFilePath{relPath: tc.entry}, } + + // recordEntry logs to the process-global logger. + runMu.Lock() s.recordEntry(relPath, info) + runMu.Unlock() require.Len(t, s.entries, 1) assert.Equal(t, tc.needsHash, s.entries[0].needsHash) -- 2.54.0