Skip to content

Commit 92ea6f9

Browse files
alnrclaude
andcommitted
fix: do not fail the suite when there is no trace to redact
The redaction added two commits ago took TestMain down with it in CI: could not redact .../playwright-traces/TestMain.zip, removing it: no such file or directory Error: remove .../TestMain.zip: no such file or directory Two mistakes. A trace that was never written cannot leak anything, so a missing archive is nothing to redact rather than an error, and removing an already-absent file is not a failure either — only a trace still on disk afterwards is worth failing over. The second mistake is what produced the missing file. Every package's TestMain traces under the same name into one shared directory, so the packages `go test ./...` runs in parallel were already overwriting each other's traces; rewriting one then raced the next writer, and a failed rewrite deleted the file out from under it. Trace names are now qualified by package, which removes the race and stops the traces from clobbering each other — worth having on its own, given these traces are the only record of what the browser did. Verified with two browser-login packages in parallel and a dummy ORY_RATE_LIMIT_HEADER: both complete, each writes its own trace (identity.TestMain.zip, relationtuples.TestMain.zip), both hold zero occurrences of the value, and the header still reaches only the console hosts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA
1 parent 45462d4 commit 92ea6f9

2 files changed

Lines changed: 36 additions & 5 deletions

File tree

cmd/cloudx/testhelpers/redact_test.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,11 @@ func TestRedactInZip(t *testing.T) {
103103

104104
assert.Error(t, redactInZip(path, secret))
105105
})
106+
107+
t.Run("case=a missing archive is not an error", func(t *testing.T) {
108+
// Tracing does not always leave a file behind, and an archive that was
109+
// never written cannot leak anything. Treating it as a failure took the
110+
// whole TestMain down with it.
111+
assert.NoError(t, redactInZip(filepath.Join(t.TempDir(), "absent.zip"), secret))
112+
})
106113
}

cmd/cloudx/testhelpers/testhelpers.go

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@ import (
88
"bytes"
99
"context"
1010
"encoding/json"
11+
"errors"
1112
"fmt"
1213
"io"
14+
"io/fs"
1315
"net/http"
1416
"net/url"
1517
"os"
@@ -355,7 +357,11 @@ func routeRateLimitHeader(t testing.TB, page playwright.Page) {
355357
// If it cannot be rewritten the trace is deleted: losing a diagnostic is the
356358
// cheaper failure by far.
357359
func stopTracing(t testing.TB, page playwright.Page) {
358-
path := filepath.Join(tracesDir, fmt.Sprintf("%s.zip", t.Name()))
360+
// The name is qualified by package because every package's TestMain traces
361+
// under the same test name into one shared directory: unqualified, the
362+
// packages `go test ./...` runs in parallel overwrite each other's traces,
363+
// and rewriting one races the next writer.
364+
path := filepath.Join(tracesDir, fmt.Sprintf("%s.%s.zip", tracesPackage, t.Name()))
359365
if err := page.Context().Tracing().Stop(path); err != nil {
360366
t.Logf("tracing stop error: %+v", err)
361367
return
@@ -365,9 +371,17 @@ func stopTracing(t testing.TB, page playwright.Page) {
365371
if !ok {
366372
return
367373
}
368-
if err := redactInZip(path, secret); err != nil {
369-
t.Logf("could not redact %s, removing it: %+v", path, err)
370-
require.NoError(t, os.Remove(path))
374+
375+
err := redactInZip(path, secret)
376+
if err == nil {
377+
return
378+
}
379+
380+
// The archive could not be rewritten, so make sure it cannot be uploaded.
381+
// Only a trace that is still on disk afterwards is worth failing over.
382+
t.Logf("could not redact %s, removing it: %+v", path, err)
383+
if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) {
384+
require.NoError(t, err, "the trace may still carry the rate-limit header")
371385
}
372386
}
373387

@@ -392,6 +406,10 @@ func redactInZip(path, secret string) error {
392406
}
393407

394408
r, err := zip.OpenReader(path)
409+
if errors.Is(err, fs.ErrNotExist) {
410+
// Tracing wrote no archive, so there is nothing that could leak.
411+
return nil
412+
}
395413
if err != nil {
396414
return err
397415
}
@@ -520,13 +538,19 @@ func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, emai
520538
}
521539
}
522540

523-
var tracesDir string
541+
var (
542+
tracesDir string
543+
// tracesPackage is the package under test, used to keep the traces of
544+
// packages running in parallel apart. See stopTracing.
545+
tracesPackage string
546+
)
524547

525548
func init() {
526549
cwd, err := os.Getwd()
527550
if err != nil {
528551
panic(err)
529552
}
553+
tracesPackage = filepath.Base(cwd)
530554
dirs := strings.Split(cwd, string(os.PathSeparator))
531555
for i := range dirs {
532556
if dirs[i] == "cloudx" {

0 commit comments

Comments
 (0)