From fb69c1b7744b8ff1ffb26166a27e9c452f803cca Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 30 May 2026 21:48:21 +0530 Subject: [PATCH] refactor(runner): one concurrent screenshot per step Move screenshot capture into the post-action errgroup so it observes the same UI moment as the hierarchy fetch. Drop the pre-action and deferred -after captures. Skip WaitForIdle when the action is Wait since the wait itself provides settling time. --- internal/runner/runner.go | 52 ++++++++++++-------------------- internal/runner/runner_test.go | 54 ++++++++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 46 deletions(-) diff --git a/internal/runner/runner.go b/internal/runner/runner.go index de944b7..7799676 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -63,8 +63,6 @@ func Run(ctx context.Context, options Options) (Summary, error) { stepIndex := 0 var lastAction *verifier.Action var lastLogTime time.Time - var pendingPostScreenshotStep int - pendingPostScreenshot := false for time.Now().Before(deadline) { if err := ctx.Err(); err != nil { break @@ -118,14 +116,10 @@ func Run(ctx context.Context, options Options) (Summary, error) { return nil }) } - if pendingPostScreenshot { - postStep := pendingPostScreenshotStep - g.Go(func() error { - captureScreenshot(gctx, options, logger, postStep, true) - return nil - }) - pendingPostScreenshot = false - } + g.Go(func() error { + captureScreenshot(gctx, options, logger, si) + return nil + }) // All goroutines write to local variables and return nil, so the Wait // error is always nil; ignored intentionally. _ = g.Wait() @@ -205,7 +199,6 @@ func Run(ctx context.Context, options Options) (Summary, error) { if err := options.TraceWriter.WriteStep(step); err != nil { return summary, fmt.Errorf("step %d trace: %w", stepIndex, err) } - captureScreenshot(ctx, options, logger, stepIndex, false) summary.Steps = stepIndex if len(violations) > 0 { summary.Violations = append(summary.Violations, ViolationRecord{ @@ -227,20 +220,17 @@ func Run(ctx context.Context, options Options) (Summary, error) { lastAction = nil } - idleCtx, idleCancel := context.WithTimeout(ctx, options.IdleTimeout) - idleErr := options.Driver.WaitForIdle(idleCtx, options.IdleTimeout) - if nextErr == nil { - pendingPostScreenshot = true - pendingPostScreenshotStep = stepIndex + // Wait actions are themselves a settling: skip the idle poll. Actions + // that mutate the UI fall through to WaitForIdle so the next step's + // concurrent fetches observe a stable post-action state. + if nextErr == nil && nextAction.Kind != verifier.ActionKindWait { + idleCtx, idleCancel := context.WithTimeout(ctx, options.IdleTimeout) + idleErr := options.Driver.WaitForIdle(idleCtx, options.IdleTimeout) + if idleErr != nil && idleCtx.Err() == nil { + logger.Warn("wait_for_idle failed", "step", stepIndex, "err", idleErr) + } + idleCancel() } - if idleErr != nil && idleCtx.Err() == nil { - logger.Warn("wait_for_idle failed", "step", stepIndex, "err", idleErr) - } - idleCancel() - } - - if pendingPostScreenshot { - captureScreenshot(ctx, options, logger, pendingPostScreenshotStep, true) } summary.EndTime = time.Now() @@ -550,23 +540,17 @@ func nextActionFromV8(ctx context.Context, web driver.WebDriver) (verifier.Actio } } -func captureScreenshot(ctx context.Context, options Options, logger *slog.Logger, stepIndex int, after bool) { +func captureScreenshot(ctx context.Context, options Options, logger *slog.Logger, stepIndex int) { image, err := options.Driver.Screenshot(ctx) if err != nil { - logger.Warn("screenshot capture failed", "step", stepIndex, "after", after, "err", err) + logger.Warn("screenshot capture failed", "step", stepIndex, "err", err) return } if len(image.PNG) == 0 { return } - var writeErr error - if after { - writeErr = options.TraceWriter.WriteScreenshotAfter(stepIndex, image.PNG) - } else { - writeErr = options.TraceWriter.WriteScreenshot(stepIndex, image.PNG) - } - if writeErr != nil { - logger.Warn("screenshot write failed", "step", stepIndex, "after", after, "err", writeErr) + if writeErr := options.TraceWriter.WriteScreenshot(stepIndex, image.PNG); writeErr != nil { + logger.Warn("screenshot write failed", "step", stepIndex, "err", writeErr) } } diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 842ee11..637369e 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -451,7 +451,10 @@ func TestRunner_ParallelFetchCallsAllDriverMethods(t *testing.T) { } } -func TestRunner_PipelinedPostScreenshotWritten(t *testing.T) { +// TestRunner_OneScreenshotPerStep verifies the runner writes a single +// screenshot per step, captured concurrently with hierarchy so the two +// observations describe the same UI moment. +func TestRunner_OneScreenshotPerStep(t *testing.T) { state := newHarness(t) state.mock.ImageData = driver.Image{PNG: []byte("fakepng"), Width: 100, Height: 200} @@ -468,24 +471,51 @@ func TestRunner_PipelinedPostScreenshotWritten(t *testing.T) { t.Fatalf("Run: %v", err) } if summary.Steps < 2 { - t.Fatalf("need at least 2 steps for pipelining test, got %d", summary.Steps) + t.Fatalf("need at least 2 steps for screenshot test, got %d", summary.Steps) } screenshotDir := filepath.Join(state.writer.Directory(), "screenshots") - - preFile := filepath.Join(screenshotDir, "step-00001.png") - if _, err := os.Stat(preFile); os.IsNotExist(err) { - t.Errorf("expected pre-screenshot for step 1: %s", preFile) + for step := 1; step <= summary.Steps; step++ { + path := filepath.Join(screenshotDir, fmt.Sprintf("step-%05d.png", step)) + if _, err := os.Stat(path); os.IsNotExist(err) { + t.Errorf("expected screenshot for step %d at %s", step, path) + } } - postFile := filepath.Join(screenshotDir, "step-00001-after.png") - if _, err := os.Stat(postFile); os.IsNotExist(err) { - t.Errorf("expected pipelined post-screenshot for step 1: %s", postFile) + entries, err := os.ReadDir(screenshotDir) + if err != nil { + t.Fatal(err) } + for _, entry := range entries { + if strings.Contains(entry.Name(), "-after") { + t.Errorf("unexpected -after screenshot remains: %s", entry.Name()) + } + } +} - lastAfter := filepath.Join(screenshotDir, fmt.Sprintf("step-%05d-after.png", summary.Steps)) - if _, err := os.Stat(lastAfter); os.IsNotExist(err) { - t.Errorf("expected flushed post-screenshot for last step %d: %s", summary.Steps, lastAfter) +// TestRunner_WaitActionSkipsIdle ensures the runner does not call WaitForIdle +// after a Wait action - the action already provides settling time. +func TestRunner_WaitActionSkipsIdle(t *testing.T) { + const waitSpec = ` +globalThis.actions = __sanderling__.actions(() => [__sanderling__.wait({ durationMillis: 5 })]); +` + state := newHarnessWithSpec(t, waitSpec) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err := Run(ctx, Options{ + Duration: 150 * time.Millisecond, + IdleTimeout: 50 * time.Millisecond, + Driver: state.mock, + Verifier: state.verifier, + TraceWriter: state.writer, + }) + if err != nil { + t.Fatalf("Run: %v", err) + } + for _, action := range state.mock.Actions() { + if action.Kind == mockdriver.ActionWaitForIdle { + t.Fatalf("Wait action must skip WaitForIdle, got: %v", action) + } } }