From 85338aeaf13ebdadf4bcf90d186b2690a81b8e1b Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 12:45:31 +0530 Subject: [PATCH] fix(runner): fail when the page reports fewer readings than extractors the comment here already claimed a partial override was fatal. it was not: the skipped check only catches indices outside the extractor list, so a page reporting values for some extractors and not others left the rest holding goja's reading of the dump with nothing said. --- internal/runner/runner.go | 11 ++++- internal/runner/web_extractor_trace_test.go | 46 +++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/internal/runner/runner.go b/internal/runner/runner.go index a777293..d730556 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -88,6 +88,7 @@ func Run(ctx context.Context, options Options) (Summary, error) { if err != nil { return Summary{}, err } + _, pageExtractors := extractorSource.(webSource) summary := Summary{StartTime: time.Now()} deadline := summary.StartTime.Add(options.Duration) @@ -219,11 +220,17 @@ func Run(ctx context.Context, options Options) (Summary, error) { }); err != nil { return summary, fmt.Errorf("step %d push: %w", stepIndex, err) } - // Both failures below leave some extractors holding the page's + // Every failure below leaves some extractors holding the page's // value and the rest holding goja's reading of the dump, and a // property comparing previous to current across that split fires - // on a healthy app. A skip also means the two engines loaded + // on a healthy app. Each also means the two engines loaded // different bundles, which nothing downstream can reconcile. + if pageExtractors && len(v8Overrides) != options.Verifier.ExtractorCount() { + return summary, fmt.Errorf( + "step %d: the page reported values for %d of the spec's %d extractors; "+ + "the page and the host are running different bundles", + stepIndex, len(v8Overrides), options.Verifier.ExtractorCount()) + } skipped, overrideErr := options.Verifier.OverrideExtractorValues(v8Overrides) if overrideErr != nil { return summary, fmt.Errorf("step %d apply extractor overrides: %w", stepIndex, overrideErr) diff --git a/internal/runner/web_extractor_trace_test.go b/internal/runner/web_extractor_trace_test.go index 0858d02..b31733e 100644 --- a/internal/runner/web_extractor_trace_test.go +++ b/internal/runner/web_extractor_trace_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "time" @@ -118,3 +119,48 @@ func TestRunner_TraceRecordsTheValueTheVerdictUsed(t *testing.T) { t.Error("no witness reached the trace; nothing was compared") } } + +// splitTableSpec registers two extractors whose goja bodies both answer "goja". +// The page below reports only the first, so index 1 keeps goja's dump-derived +// reading while index 0 holds the page's. +const splitTableSpec = ` +import { actions, extract } from "@sanderling/spec"; +extract("first", () => "goja"); +extract("second", () => "goja"); +globalThis.properties = {}; +globalThis.actions = actions(() => []); +` + +// TestRunner_PartialExtractorTableIsFatal pins the failure the runner used to +// let through. JSON.stringify drops an undefined-valued key, so a page whose +// extractors are mostly undefined off their own screen reported a table with +// holes in it, and the run completed with half the extractors reading from V8 +// and half from goja. A delta property spanning that split convicts an app that +// did nothing wrong, which is worse than a crash: it is a green report of a bug +// that is not there, or a red one for a bug nobody can reproduce. +func TestRunner_PartialExtractorTableIsFatal(t *testing.T) { + state := newHarnessWithSpec(t, splitTableSpec) + web := &webMockDriver{ + Driver: state.mock, + overrides: map[int]json.RawMessage{0: json.RawMessage(`"v8"`)}, + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err := Run(ctx, Options{ + Duration: time.Hour, + IdleTimeout: 20 * time.Millisecond, + MaxSteps: 2, + Driver: web, + Verifier: state.verifier, + TraceWriter: state.writer, + }) + if err == nil { + t.Fatal("the run completed on a page that reported 1 of 2 extractors; " + + "the second extractor silently kept goja's value") + } + const want = "the page reported values for 1 of the spec's 2 extractors" + if !strings.Contains(err.Error(), want) { + t.Errorf("Run failed with %q, want it to name the split: %q", err, want) + } +}