chore: try to fix x-list-waterfall flaky test #1474
Conversation
|
📝 WalkthroughWalkthroughReduced the x-list-waterfall test size: the test name changed to "x-list-waterfall-100" and the number of created Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html (1)
32-43: Remove randomness to deflake metrics: make item heights deterministicRandom heights can subtly alter layout passes and keep the test flaky. Use a deterministic height derived from the index; this preserves variability without randomness.
Apply this diff:
- function getRandomHeight() { - return Math.floor(Math.random() * 151) + 150; // Generates random height between 150 and 300 (inclusive) - } + function getHeightForIndex(i) { + // 150..300 inclusive, deterministic by index + return 150 + (i % 151); + } @@ - `--item-index: ${ii}; height: ${getRandomHeight()}px;`, + `--item-index: ${ii}; height: ${getHeightForIndex(ii)}px;`,
🧹 Nitpick comments (1)
packages/web-platform/web-tests/tests/performance.test.ts (1)
176-176: Decouple test title from fixture name to avoid future breakagePassing the title ties file lookup to the test name. Use an explicit fixture string like other cases in this file.
- const cdpSession = await goto({ page, browserName, context }, title); + const cdpSession = await goto({ page, browserName, context }, 'x-list-waterfall-100');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web-platform/web-tests/tests/performance.test.ts(1 hunks)packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.627Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts:95-99
Timestamp: 2025-07-16T05:57:29.837Z
Learning: In the lynx-stack codebase, PupilTong prefers avoiding regex for parsing in performance-critical code paths like SSR hydration, preferring simple string manipulation operations even if they're less robust, when the input format is predictable and controlled.
🔇 Additional comments (2)
packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html (1)
36-49: LGTM on scaling down to 100 itemsReducing to 100 aligns with the new test title and should help with CI stability due to lower work per run.
packages/web-platform/web-tests/tests/performance.test.ts (1)
174-181: LGTM: Test Title Matches FixtureConfirmed that the HTML fixture and the test title are in sync—no further changes needed.
- Fixture exists at
packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html- Test invocation in
packages/web-platform/web-tests/tests/performance.test.tson line 174 correctly uses'x-list-waterfall-100'
CodSpeed Performance ReportMerging #1474 will not alter performanceComparing Summary
|
React Example#4160 Bundle Size — 237.04KiB (0%).3906013(current) vs a77e659 main#4159(baseline) Bundle metrics
|
| Current #4160 |
Baseline #4159 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
0% |
38.47% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
158 |
158 |
| Duplicate Modules |
64 |
64 |
| Duplicate Code |
45.86% |
45.86% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4160 |
Baseline #4159 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.28KiB |
91.28KiB |
Bundle analysis report Branch PupilTong:p/hw/fix-x-waterfall-1... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#4156 Bundle Size — 344.35KiB (0%).3906013(current) vs a77e659 main#4155(baseline) Bundle metrics
|
| Current #4156 |
Baseline #4155 |
|
|---|---|---|
| Initial JS |
143.29KiB |
143.29KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
7 |
7 |
| Assets |
7 |
7 |
| Modules |
211 |
211 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
3.96% |
3.96% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4156 |
Baseline #4155 |
|
|---|---|---|
| JS |
229.55KiB |
229.55KiB |
| Other |
82.95KiB |
82.95KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch PupilTong:p/hw/fix-x-waterfall-1... Project dashboard
Generated by RelativeCI Documentation Report issue
PupilTong
force-pushed
the
p/hw/fix-x-waterfall-10000-flaky
branch
from
b69f98f to
0d80382
Compare
❌ 2 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
PupilTong
force-pushed
the
p/hw/fix-x-waterfall-10000-flaky
branch
from
0d80382 to
54f2cde
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html (2)
32-44: Eliminate randomness to remove nondeterminism in performance metricsMath.random() causes non-reproducible layout/style counts and can keep the test flaky even after reducing item count. Make heights deterministic from the index.
Apply:
- function getRandomHeight() { - return Math.floor(Math.random() * 151) + 150; // Generates random height between 150 and 300 (inclusive) - } + function getDeterministicHeight(i) { + // 150..300 inclusive, stable per index + return 150 + (i % 151); + } @@ - for (let ii = 0; ii < 100; ii++) { + for (let ii = 0; ii < 100; ii++) { const listItem = document.createElement('list-item'); listItem.setAttribute('item-key', ii); listItem.setAttribute( 'style', - `--item-index: ${ii}; height: ${getRandomHeight()}px;`, + `--item-index: ${ii}; height: ${getDeterministicHeight(ii)}px;`, );Also applies to: 37-49
36-50: Stabilize upgrade timing of custom elements and reduce reflow churnIf x-list/x-view aren’t defined when items are appended, upgrade timing can vary and affect LayoutCount/RecalcStyleCount. Also, appending one-by-one may trigger extra style/layout work.
Consider:
- queueMicrotask(() => { - for (let ii = 0; ii < 100; ii++) { - const listItem = document.createElement('list-item'); - listItem.setAttribute('item-key', ii); - listItem.setAttribute( - 'style', - `--item-index: ${ii}; height: ${getDeterministicHeight(ii)}px;`, - ); - listItem.setAttribute('class', 'item'); - const xview = document.createElement('x-view'); - xview.innerHTML = ii; - listItem.append(xview); - list.append(listItem); - } - }); + queueMicrotask(async () => { + // Ensure custom elements are upgraded before populating + await Promise.allSettled([ + customElements.whenDefined?.('x-list'), + customElements.whenDefined?.('x-view'), + ]); + const frag = document.createDocumentFragment(); + for (let ii = 0; ii < 100; ii++) { + const listItem = document.createElement('list-item'); + listItem.setAttribute('item-key', ii); + listItem.setAttribute( + 'style', + `--item-index: ${ii}; height: ${getDeterministicHeight(ii)}px;`, + ); + listItem.setAttribute('class', 'item'); + const xview = document.createElement('x-view'); + xview.textContent = String(ii); + listItem.append(xview); + frag.append(listItem); + } + list.append(frag); + });
- Awaiting definitions removes race with the deferred library script.
- Using a fragment batches DOM ops, reducing layout/style recomputation.
🧹 Nitpick comments (1)
packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html (1)
46-47: Prefer textContent over innerHTML for plain textAvoids unnecessary HTML parsing and is marginally faster/safer.
- xview.innerHTML = ii; + xview.textContent = String(ii);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web-platform/web-tests/tests/performance.test.ts(1 hunks)packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/web-platform/web-tests/tests/performance.test.ts
🔇 Additional comments (1)
packages/web-platform/web-tests/tests/web-elements/performance/x-list-waterfall-100.html (1)
37-37: x-list-waterfall-100 thresholds updated and naming consistent
- In packages/web-platform/web-tests/tests/performance.test.ts (lines 174–180), the
x-list-waterfall-100test now asserts
• LayoutCount ≤ 7
• RecalcStyleCount ≤ 8
which aligns with the reduced 100-item workload.- The HTML fixture (
tests/web-elements/performance/x-list-waterfall-100.html) and the test title (x-list-waterfall-100) are in sync.No further changes required.
PupilTong
force-pushed
the
p/hw/fix-x-waterfall-10000-flaky
branch
from
54f2cde to
3906013
Compare
Summary by CodeRabbit
Checklist