test(demo): add bisect seed history - #99
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| browser: 'chromium', | ||
| args: ['--no-sandbox'], | ||
| }, | ||
| viewports: ['phone'], |
There was a problem hiding this comment.
This scopes visreg down to viewports: ['phone'] (dropping desktop/tablet), and below perf drops to phone-only too, plus numberOfMeasurements goes 10→8. These are global config changes that apply to every AB run for this demo, not just bisect runs — worth confirming that's intentional. If it's purely to keep bisect (which rebuilds per-commit) fast, consider scoping it to the bisect category only if the config surface supports per-category viewport overrides, so regular (non-bisect) visreg/perf runs keep full desktop/tablet coverage and full measurement count. Otherwise this is a real, unstated reduction in demo test coverage/statistical power.
| const configPath = path.resolve( | ||
| __dirname, | ||
| '../../../../../../demo-ecommerce/abtests.config.ts', | ||
| ); | ||
|
|
||
| expect(fs.readFileSync(configPath, 'utf8')).toContain( |
There was a problem hiding this comment.
This test does a raw substring match against abtests.config.ts's source text (via a path.resolve with six ../ segments). It's brittle in two ways: (1) any harmless reformatting of that line (quote style, whitespace, splitting the && command) breaks the test without an actual behavior regression; (2) the deep relative path silently breaks if this test file or the config ever move. Consider importing/parsing the resolved config (or at least matching a looser regex on the meaningful parts: rm -rf public/packs tmp/cache and assets:precompile) instead of an exact literal match.
| status: 'running', | ||
| goodSha: seedCommits[0], | ||
| badSha: seedCommits.at(-1)!, | ||
| originalExperiment: { sha: seedCommits.at(-1)!, branch: 'codex/git-bisect-demo-history' }, |
There was a problem hiding this comment.
Minor: branch: 'codex/git-bisect-demo-history' doesn't match this PR's actual branch (ramez/bisect-demo-history). Looks like leftover copy-paste from an earlier draft branch name — harmless since the assertions don't depend on it, but worth cleaning up so it doesn't confuse someone tracing this fixture back to a real branch later.
| checksum += Math.sqrt(checksum + 1); | ||
| } | ||
|
|
||
| if (checksum === Number.POSITIVE_INFINITY) { |
There was a problem hiding this comment.
checksum === Number.POSITIVE_INFINITY is effectively unreachable (a ~450ms sum of sqrt increments can't overflow to Infinity), so this is dead code kept only to dodge dead-code elimination of the busy-loop. That's fine as a fixture technique, but as written it reads like a real conditional. A one-line comment explaining "kept to prevent the loop from being optimized away" would save the next reader from treating this as a real check. Same pattern in ProductDetailPage.tsx.
ReviewOverviewThis side PR seeds a deterministic, bisectable commit history on the demo-ecommerce app: four category-specific regression fixtures (visreg on the homepage hero, perf CPU-warmup on homepage + product detail, a11y offscreen unnamed button), plus a What's solid
Issues raised inline
Not verifiedI wasn't able to run SecurityNo concerns — all changes are demo-app fixture code and a fixed (non-user-controlled) shell command string in the rebuild config; no injection surface. |
Greptile SummaryThis PR adds deterministic demo history for category-specific bisect testing. The main changes are:
Confidence Score: 4/5The phone-only visual configuration drops existing coverage and should be corrected before merging.
demo-ecommerce/abtests.config.ts; packages/shaka-perf/src/compare/bisect/tests/seed-history.test.ts Important Files Changed
Reviews (1): Last reviewed commit: "test(demo): configure bisect acceptance ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| browser: 'chromium', | ||
| args: ['--no-sandbox'], | ||
| }, | ||
| viewports: ['phone'], |
There was a problem hiding this comment.
The viewport planner intersects this global list with each test's viewport list. With only phone enabled, desktop-only Admin tests and the tablet/desktop Product Detail Actions test produce no visual work, so regressions in those tests disappear instead of remaining unaffected.
| const candidateIndex = seedCommits.indexOf(work.sha); | ||
| const regressionsPresent = new Set( | ||
| seedCommits | ||
| .slice(0, candidateIndex + 1) | ||
| .flatMap( | ||
| (sha) => seedRegressionsByCommit[sha as keyof typeof seedRegressionsByCommit] ?? [], | ||
| ), |
There was a problem hiding this comment.
Regression presence is generated from the same hard-coded commit list and map used to define the expected answers. If a seed commit is removed, reordered, or no longer contains its documented page change, this test still passes while a real Git bisect reports a different first-bad commit.
ReviewThis PR adds a deterministic, bisectable demo commit history to Overall: the regression fixtures are well-isolated (contained to Home/ProductDetail, deterministic CPU busy-loops instead of network delays, no data/API changes) and the seed-history doc is a nice touch for future maintainers. The unit test exercises the actual A few things worth a look (left as inline comments):
Also noting for awareness (not blockers): 🤖 Generated with Claude Code |
| '38dae6871b8b443dd1880269dacde951700e77cc', | ||
| '623a1ae6f39cb6cbddd550ecd33d83c670877d14', | ||
| 'fe8900e2568f11d05e41d43ad62e0aa863017264', | ||
| '58cc828b7272cd69408fa4dc5cd36206dcd8846a', | ||
| 'a55e7f44dea86afa94775cf448dc3e696c71ed33', | ||
| '754fcd9b72b5623118a9bc1fb84b87ec98e002e1', | ||
| '9c7cfff6c0ca9bd561f5bb9905a1b09ee3132d1e', | ||
| '744fe902ab6b087f761f2df7e1f52bcc2e88d1c1', | ||
| 'ac38e5320e6c33b67474f445c15e3012e22fb491', | ||
| '463c4296e003fafe5dc4e23f5f43e805e555938d', | ||
| 'fcb0e2b107a99c6e4edab01da114d4d83b3d7a94', | ||
| 'c1e2a62486870b02354c0c5b8727f8944e4913a3', | ||
| 'ce1f6015bfd01e05228d94affb788fe5f0d896a0', | ||
| '3846371172486d851b836883c54520cc1b844199', | ||
| '5345dffb62b761b9cb0e1516a6bbd4389a6cf642', | ||
| '088afb9342d8c4337361df177e3731550b096fc9', | ||
| '4406a7800cfec9af52e9f7e731a1ad59915ac227', | ||
| 'f7b872f2a6d5817be15261b4d9f21a4f6814126f', |
There was a problem hiding this comment.
This array has 18 commits (the base + 17), but docs/git-bisect-seed-history.md's "Category map" only documents 16 commits after the base (623a1ae … 4406a78). The last entry here, f7b872f2a6d5817be15261b4d9f21a4f6814126f, doesn't appear in the doc at all.
It's used as badSha/the end of orderedCommits, so it doesn't affect the test's assertions today, but it means the doc is no longer a complete map of the fixture branch — worth adding a line for it (or dropping it if it's stray) so the two stay in sync for whoever bisects this branch by hand later.
| status: 'running', | ||
| goodSha: seedCommits[0], | ||
| badSha: seedCommits.at(-1)!, | ||
| originalExperiment: { sha: seedCommits.at(-1)!, branch: 'codex/git-bisect-demo-history' }, |
There was a problem hiding this comment.
branch: 'codex/git-bisect-demo-history' doesn't match this PR's actual head branch (ramez/bisect-demo-history). It's inert test data (nothing asserts on it), but it reads like a leftover from a different branch/tooling context and could mislead someone debugging this fixture later.
| it('clears persistent build output before precompiling each candidate', () => { | ||
| const configPath = path.resolve( | ||
| __dirname, | ||
| '../../../../../../demo-ecommerce/abtests.config.ts', | ||
| ); | ||
|
|
||
| expect(fs.readFileSync(configPath, 'utf8')).toContain( | ||
| "command: 'rm -rf public/packs tmp/cache && SECRET_KEY_BASE_DUMMY=1 ./bin/rails assets:precompile'", | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test asserts an exact substring of the raw abtests.config.ts source, including quote style and whitespace ("command: 'rm -rf public/packs tmp/cache && ..."). A harmless reformat (prettier, switching quote style, wrapping the line) would fail this test with no behavior change.
Consider asserting against the loaded/parsed config value (e.g. import the resolved bisect.rebuildCommands array and check an entry's command) instead of grepping file text — same coverage, but resilient to formatting.
| it('finds the documented first bad commit for every seeded regression target', () => { | ||
| let session = seedSession(); | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
while (true) with no iteration cap: if a future change to search.ts introduces a bug where nextCandidate never returns null for this fixture (e.g. an off-by-one that keeps splitting forever), this test hangs until Jest's default timeout instead of failing with a clear assertion. A for (let i = 0; i < seedCommits.length * 4; i++) guard (or similar) with a fail('did not converge') after would make that failure mode diagnosable.
1 participant
PR 5 of 12; side branch from PR 4.
Summary
Why
Provide deterministic demo history for validating category-specific bisect behavior without coupling fixture changes to the product stack.
Checks
yarn workspace shaka-perf typecheck