Skip to content

Refactor: consolidate duplicated concepts between perf and visreg code paths #12

Description

@Romex91

Context

From the fix in 215fb79 ("visreg: narrow scenario.viewports from options.viewports"): per-test viewport narrowing had three independent implementations (perf in run.ts, visreg inline in convertAbTestToScenario.ts, harvest-time filter in run.ts). Before adding more parallel implementations, here's a catalog of duplication between the two domains, verified against the current tree.

Each item lists the concept, file refs, whether it's exact or parallel duplication, and where a shared implementation would naturally live.


1. Viewport handling

1a. Narrow category viewports by test options.viewports (highest impact)

  • packages/shaka-perf/src/compare/run.ts:76-84resolveViewportsForTest
  • packages/shaka-perf/src/visreg/core/util/convertAbTestToScenario.ts:61-67 — inline filter (added in 215fb79)
  • packages/shaka-perf/src/compare/run.ts:400-401 — harvest-time filter (same rule, used as defence-in-depth)

Pattern: Exact duplication of new Set(narrow).has(v.label) across three call sites.
Shared home: shaka-sharedresolveViewportsForTest(test, categoryViewports): Viewport[]. All three call sites collapse to categoryViewports.filter(v => allowedLabels.has(v.label)) with the allowed set coming from the shared helper.


2. Test discovery & filtering

2a. Per-engine loadTests invocation with TestType gating

  • packages/shaka-perf/src/compare/run.ts:159-163 — loads untyped, then filters per-test via testRunsForType
  • packages/shaka-perf/src/bench/cli/commands/compare/index.ts:120-125 — calls loadTests with TestType.Performance pre-filter
  • packages/shaka-perf/src/visreg/core/util/createComparisonBitmaps.ts:40-45 — calls loadTests with TestType.VisualRegression pre-filter

Pattern: Parallel but slightly different. The outer compare runner loads once; each engine also re-loads itself. Adding a new TestType (e.g. Accessibility) currently requires touching both engines.
Shared home: Push the full test list from compare into each engine-bridge rather than having engines re-load. invokePerfEngine / invokeVisregEngine accept a pre-filtered tests: AbTestDefinition[] and skip loadTests internally.

2b. Bench filter regex composition

  • packages/shaka-perf/src/compare/run.ts:237-241 — per-name anchored regex, comma-joined, with special-char escape
  • Similar regex-escape mindset at packages/shaka-perf/src/bench/cli/commands/compare/index.ts:143

Pattern: Parallel; both escape regex meta-characters in test names for different purposes (filtering vs. slugifying).
Shared home: Minor — compare/util/test-name.ts colocating escapeForBenchFilter and slugifyTestName.


3. URL composition

3a. Build full test URL from startingPath + baseURL

  • packages/shaka-perf/src/compare/run.ts:496-497
  • packages/shaka-perf/src/visreg/core/util/convertAbTestToScenario.ts:16-17
  • packages/shaka-perf/src/bench/core/create-lighthouse-benchmark-in-process.ts:88-95 (richer — preserves baseUrl query params)

Pattern: Same concept, three impls. Only the bench variant forwards query strings from baseUrl, so phone/tablet base URLs with ?hydration_delay=… currently work for perf but silently drop on the other two paths.
Shared home: shaka-shared/util/url.tscomposeTestUrl(startingPath, baseUrl) that unconditionally preserves query params. That also closes the query-drop inconsistency.

3b. URL host → filename segment (host.replace(':', '_'))

  • packages/shaka-perf/src/bench/core/html-diff.ts:7
  • packages/shaka-perf/src/compare/harvest/perf.ts:292
  • packages/shaka-perf/src/bench/cli/commands/compare/index.ts:225-226

Pattern: Exact duplication — three sites, identical one-liner to produce a filename-safe host segment.
Shared home: compare/util/artifact.tshostSegment(url: string).


4. Artifact layout

4a. Test-name slug

  • packages/shaka-perf/src/compare/harvest/perf.ts:58-60 — exported slugifyForBench
  • packages/shaka-perf/src/bench/cli/commands/compare/index.ts:143 — inline re-implementation of the same regex

Pattern: Exact duplication of name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-|-$/g, '').
Shared home: Either lift slugifyForBench into shaka-shared or have the bench command import it from compare/harvest/perf.


5. Engine-side temp config writing

5a. writeTempConfig + cleanup pattern

  • packages/shaka-perf/src/compare/engine-bridge/perf.ts:89-90 — Lighthouse config
  • packages/shaka-perf/src/compare/engine-bridge/visreg.ts:53-54 — visreg config

Pattern: Parallel — both do crypto.randomBytes(6).toString('hex') hash suffixes under os.tmpdir() with shaka-perf-<flavour>-<hash>.js naming and matching fs.rmSync(..., { force: true }) cleanup in a finally.
Shared home: compare/util/temp-file.tswithTempConfig(prefix, payload, fn) that writes, runs, and cleans up.


6. Engine error surfaces

6a. Engine-error file conventions

  • packages/shaka-perf/src/bench/cli/commands/compare/index.ts:28-29 — writes engine-error.txt + engine-output.log per-test
  • packages/shaka-perf/src/compare/harvest/perf.ts — reads them back through readPerfEngineError / readPerfEngineLog
  • Visreg writes errors inline into its report.json (different convention entirely)

Pattern: Parallel file conventions for perf; visreg goes through its manifest. Worth unifying only when visreg gets per-pair error cards in the report shell.
Shared home: Defer.


7. Already centralized (don't re-implement)

  • Status union at compare/report.ts:4-19 and combineStatus at compare/run.ts:107-123 — single source of truth across categories.
  • Config validation in compare/config.ts — Zod schemas with cross-category label checks already cover both sides.
  • skippedCategory() helper at compare/run.ts:353-368 — used for both testTypes opt-outs and viewport-narrow empty intersections.

8. Explicitly different, not duplication

  • Retry loops — visreg has retryCompare.ts with re-screenshot + re-navigation; perf uses statistical variance instead. Different semantics.
  • Page-settle waits — visreg-only; bench uses Lighthouse's internal gather loop.
  • Metric classification / formatting — perf-only; visreg compares images, not metrics.

Recommended order

  1. 1a (viewport narrow) — most duplicated, simplest to extract, already on the critical path.
  2. 3a + 3b (URL / host helpers) — trivial three-way extractions; 3a also fixes a silent query-string drop inconsistency.
  3. 4a (slug) — one-line extraction.
  4. 5a (temp config) — small, useful when we add more engines.
  5. 2a (engine test-loading contract) — larger, touches engine-bridge shape.
  6. 2b + 6a — defer.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions