fix(web): read the page's extractors only on steps that count

the page advances the spec's carriers when it evaluates, but the runner
applied the result only on non-transitional steps. a discarded step
moved the window forward anyway, so the next accepted pair bracketed two
transactions while counting one submit, and convicted a healthy app.
extractor errors now fail the run instead of leaving goja's values in
current against v8's in previous.
This commit is contained in:
pj committed 2026-08-14 22:48:38 +05:30
1 parent b1bf39bac5
commit f738a590f0
2 files changed
+249 -19

No files matched your search

+53 -19
View File
@@ -74,6 +74,7 @@ func Run(ctx context.Context, options Options) (Summary, error) {
if logger == nil {
logger = slog.Default()
}
options.IdleTimeout = resolveIdleTimeout(options)
// Gate on the app actually being on top before acting, so the first
// action never fires against a leftover screen or a system dialog. Done
@@ -122,9 +123,8 @@ func Run(ctx context.Context, options Options) (Summary, error) {
var logs []verifier.LogEntry
// gctx is bound to the errgroup so a returned error (or outer
// cancellation) propagates to siblings - notably the V8 extractor
// goroutine, whose CDP round-trip can otherwise outrun the step
// budget on a hung tab.
// cancellation) propagates to every sibling read rather than leaving
// one blocked on a hung device.
g, gctx := errgroup.WithContext(ctx)
si := stepIndex
// fetchSyncedState issues a single Snapshot RPC so hierarchy and
@@ -143,19 +143,6 @@ func Run(ctx context.Context, options Options) (Summary, error) {
logs = collectLogs(gctx, options.Driver, logSince)
return nil
})
var v8Overrides map[int]json.RawMessage
// The same lastAction PushSnapshot hands the goja state below: the two
// engines evaluate this step against one action, not two.
stepAction := lastAction
g.Go(func() error {
overrides, err := extractorSource.ExtractorOverrides(gctx, stepAction)
if err != nil {
logger.Warn("v8 extractor evaluation failed", "step", si, "err", err)
return nil
}
v8Overrides = overrides
return nil
})
// All goroutines write to local variables and return nil, so the Wait
// error is always nil; ignored intentionally.
_ = g.Wait()
@@ -198,6 +185,29 @@ func Run(ctx context.Context, options Options) (Summary, error) {
var witnesses map[string]trace.Witness
skippedVerification := false
if !transitional {
// The page-side extractors evaluate only on steps the verifier will
// accept, which is why this read waits for the tree instead of
// racing it. A spec's extractor getters carry state across steps
// (folio's last-seen Home total, its submit counters) and that state
// advances every time they run: evaluating them on a step whose
// values are then thrown away leaves the page one window ahead of
// the verifier, so the next accepted pair brackets two committed
// transactions while having counted one submit, and the property
// convicts a healthy app. It costs the latency the read used to hide
// behind the hierarchy fetch; the fetch is what decides whether this
// step counts at all, so it has to go first.
//
// lastAction is the same value PushSnapshot hands the goja state
// below: the two engines evaluate this step against one action.
v8Overrides, overridesErr := extractorSource.ExtractorOverrides(ctx, lastAction)
if overridesErr != nil {
// Not a warning. Without the page's values this step's
// extractors keep goja's dump-derived readings while the
// previous step holds the page's, and a delta property then
// compares two producers and fires on an app that did nothing
// wrong.
return summary, fmt.Errorf("step %d extractor overrides: %w", stepIndex, overridesErr)
}
if err := options.Verifier.PushSnapshot(verifier.SnapshotInput{
Tree: tree,
ScreenshotPNG: screenshotPNG,
@@ -384,12 +394,36 @@ func validate(options Options) error {
if options.Duration <= 0 {
return errors.New("runner: Duration must be positive")
}
if options.IdleTimeout <= 0 {
options.IdleTimeout = 2 * time.Second
}
return nil
}
// defaultIdleTimeout is the settle budget a caller that names none gets.
const defaultIdleTimeout = 2 * time.Second
// idleTimeoutFloor is a driver that knows how long its own settle can take.
// Declared here rather than in the driver package (like lastActionInstaller in
// source.go) so the mobile drivers stay untouched.
type idleTimeoutFloor interface {
MinIdleTimeout() time.Duration
}
// resolveIdleTimeout settles the per-step settle budget: the caller's value,
// defaulted when unset, and raised to whatever the driver says its own settle
// needs. The chrome driver's settle waits for the DOM to go quiet and only then
// opens its route-transition window; handed less than their sum it is cut off
// mid-transition, and the step samples the screen the app is leaving. A driver
// that reports no floor keeps the caller's value exactly.
func resolveIdleTimeout(options Options) time.Duration {
timeout := options.IdleTimeout
if timeout <= 0 {
timeout = defaultIdleTimeout
}
if floor, ok := options.Driver.(idleTimeoutFloor); ok {
timeout = max(timeout, floor.MinIdleTimeout())
}
return timeout
}
// ensureForeground keeps the app under test in the foreground. When the driver
// can report the foreground app and it no longer matches the bundle under test,
// the app is relaunched. Returns true when a relaunch happened so the caller