chore(backstop): tweak parameters to reduce false positives - #8041
Conversation
WalkthroughBackstop config switched from Puppeteer to Playwright, added scenarioDefaults and per-relative-URL conditional delay handling, expanded engineOptions and resembleOutputOptions, and reduced asyncCaptureLimit to 1; dozens of SCSS files and one build script have non-functional CSS or import-order reorderings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
|
Preview: https://pf-pr-8041.surge.sh A11y report: https://pf-pr-8041-a11y.surge.sh |
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @backstop.js:
- Around line 42-49: The config currently forces CPU rendering by including
'--disable-gpu' in the args array, which reduces speed but improves determinism
alongside asyncCaptureLimit: 1 and misMatchThreshold: 0.001; make this explicit
and configurable by replacing the hardcoded '--disable-gpu' in the args array
with a conditional inclusion controlled by an environment variable (e.g.,
BACKSTOP_DISABLE_GPU) or a top-level config flag, add a short inline comment
near the args array explaining the trade-off, and ensure CI pipeline runtime is
validated when the flag is enabled.
- Line 58: The change sets asyncCaptureLimit to 1 which forces serial screenshot
captures (alongside the delay increase to 250ms per scenario), potentially
inflating test runtime; to mitigate, adjust asyncCaptureLimit back to a small
parallel value (e.g., 2 or 3) in backstop configuration (look for the
asyncCaptureLimit setting) to balance parallelism and stability, then run CI to
measure runtimes and iterate—if flakiness persists, keep at 1 but document the
rationale and monitor total test duration.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
backstop.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-upload
🔇 Additional comments (3)
backstop.js (3)
15-15: Good change, but monitor effectiveness and consider reverting if unnecessary.Increasing the delay from 100ms to 250ms provides additional time for page rendering and should help reduce rendering-related false positives. Since the PR description indicates this may be reversible, track false positive rates after deployment to determine if the full 250ms is needed or if a smaller value (e.g., 150ms or 200ms) would suffice.
60-63: resembleOutputOptions configuration is valid for BackstopJS 6.3.25.Both options are correctly supported:
errorType: 'movementDifferenceIntensity'is a standard Resemble.js error type for highlighting motion-like differencesignoreAntialiasing: trueis a valid option for reducing false positives from antialiasing artifactsThe configuration will effectively reduce noise from diagonal lines and curved elements in visual comparisons.
26-27: No action needed—these settings operate at different levels and are not redundant.The viewport-level
deviceScaleFactor: 1and the Chrome flag--force-device-scale-factor=1serve complementary purposes:
deviceScaleFactor: 1is a per-page Puppeteer DevTools Protocol emulation that controls CSS pixel ↔ device pixel reporting within each page.--force-device-scale-factor=1is a global Chromium browser-level setting that affects display scaling, font rasterization, and the entire rendering pipeline.They operate at different architectural levels and are not redundant.
Likely an incorrect or invalid review comment.
| args: [ | ||
| '--no-sandbox', | ||
| '--disable-setuid-sandbox', | ||
| '--force-device-scale-factor=1', | ||
| '--font-render-hinting=none', | ||
| '--disable-font-subpixel-positioning', | ||
| '--disable-gpu' | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find backstop.js file
fd -type f -name "backstop.js"Repository: patternfly/patternfly
Length of output: 235
🏁 Script executed:
# Find backstop.js file
fd -t f -name "backstop.js"Repository: patternfly/patternfly
Length of output: 295
🏁 Script executed:
# Find backstop.js using git
git ls-files | grep backstop.jsRepository: patternfly/patternfly
Length of output: 76
🏁 Script executed:
# Read backstop.js to examine lines 42-49 and surrounding context
cat -n backstop.js | head -70Repository: patternfly/patternfly
Length of output: 2722
🏁 Script executed:
# Search for any comments or documentation about these Chrome flags
rg "disable-gpu|font-render-hinting|disable-font-subpixel" --context 3Repository: patternfly/patternfly
Length of output: 392
🏁 Script executed:
# Search for any documentation about backstop configuration or visual testing
rg "backstop|visual.*test|rendering.*consist" -i --max-count 5 README* CONTRIBUTING* docs/* 2>/dev/null | head -30Repository: patternfly/patternfly
Length of output: 47
🏁 Script executed:
# Search for any documentation or comments related to backstop or visual testing
find . -type f -name "*.md" | head -10Repository: patternfly/patternfly
Length of output: 445
🏁 Script executed:
# Search README for backstop or visual testing information
rg "backstop|visual.*test" README.md -i -A 3 -B 3Repository: patternfly/patternfly
Length of output: 47
🏁 Script executed:
# Search for git history of backstop.js to see when --disable-gpu was added
git log --oneline backstop.js | head -10Repository: patternfly/patternfly
Length of output: 125
🏁 Script executed:
# Get details about the commit that modified backstop.js
git show d0691d6f backstop.jsRepository: patternfly/patternfly
Length of output: 2638
Acknowledge the performance trade-off with --disable-gpu in visual regression tests.
The --disable-gpu flag forces CPU-based rendering instead of GPU acceleration, which trades rendering speed for consistency. Combined with asyncCaptureLimit: 1, this appears to be an intentional design to ensure deterministic visual regression testing—consistent with the strict misMatchThreshold: 0.001 and the font-rendering flags (--font-render-hinting=none, --disable-font-subpixel-positioning).
This is a reasonable approach for visual regression testing where consistency matters more than speed, but verify that CI/CD execution time is acceptable for your testing workflow.
🤖 Prompt for AI Agents
In @backstop.js around lines 42-49, The config currently forces CPU rendering by
including '--disable-gpu' in the args array, which reduces speed but improves
determinism alongside asyncCaptureLimit: 1 and misMatchThreshold: 0.001; make
this explicit and configurable by replacing the hardcoded '--disable-gpu' in the
args array with a conditional inclusion controlled by an environment variable
(e.g., BACKSTOP_DISABLE_GPU) or a top-level config flag, add a short inline
comment near the args array explaining the trade-off, and ensure CI pipeline
runtime is validated when the flag is enabled.
mcoker
left a comment
There was a problem hiding this comment.
LGTM! From testing, looks like it takes about twice as long to run now. If we want to try reducing that at some point, we can try bumping asyncCaptureLimit and reducing the delay
d0691d6 to
173a7cd
Compare
|
I was still getting artifacts that looked to be from partially loaded pages, so I made a couple of changes:
Diffs look much better in playwright! I didn't see any more partially loaded pages/images, and no random dark theme elements on pages. I still see some diagonal/slanted diffs, but where I probably got 20 or so using puppeteer (plus a bunch of other partially loaded/dark-theme diffs), there were 7 in playwright. Plus the overall run takes less time (about 52 minutes compared to 56 minutes or so with a full run), even with the new PDF report - https://drive.google.com/file/d/1KACVVOaxevAs4nSHPr3UcdTtc5IgrwdQ/view?usp=sharing FWIW this is the list of slanted diffs that I spotted
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
src/patternfly/components/CodeEditor/code-editor.scsssrc/patternfly/components/Drawer/drawer.scsssrc/patternfly/components/ExpandableSection/expandable-section.scsssrc/patternfly/components/Form/form.scsssrc/patternfly/components/FormControl/form-control.scsssrc/patternfly/components/Label/label-group.scsssrc/patternfly/components/Masthead/masthead.scsssrc/patternfly/components/ModalBox/modal-box.scsssrc/patternfly/components/Page/page.scsssrc/patternfly/components/Sidebar/sidebar.scsssrc/patternfly/components/Tabs/tabs.scsssrc/patternfly/components/Wizard/wizard.scsssrc/patternfly/sass-utilities/mixins.scss
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T15:25:08.994Z
Learnt from: srambach
Repo: patternfly/patternfly PR: 8030
File: src/patternfly/components/TreeView/tree-view.scss:441-445
Timestamp: 2025-12-17T15:25:08.994Z
Learning: In PatternFly TreeView component (src/patternfly/components/TreeView/tree-view.scss), the `.pf-m-disabled` modifier on list items affects only the visual appearance of node content (text and icons), but intentionally does not change the toggle button color. The toggle remains functional and visually distinct to indicate the item can still be expanded/collapsed.
Applied to files:
src/patternfly/components/ExpandableSection/expandable-section.scss
🔇 Additional comments (12)
src/patternfly/components/CodeEditor/code-editor.scss (1)
95-95: Non-functional declaration reordering.This change simply repositions the
flex-direction: column;declaration within the same selector without altering its value or behavior.However, note that the PR objectives describe Backstop configuration changes (Chromium flags, Playwright migration, Resemble options), but this file contains only SCSS styling adjustments. Please verify whether these SCSS changes are intentionally part of this PR or were inadvertently included.
src/patternfly/components/Sidebar/sidebar.scss (1)
181-181: LGTM—non-functional declaration reordering.The
flex-basisproperty was repositioned within the.#{$sidebar}__panelselector without changing its value or runtime behavior.src/patternfly/components/Wizard/wizard.scss (1)
354-354: LGTM—non-functional declaration reorderings.All three instances of
flex-direction: column;(lines 354, 367, 594) were repositioned within their respective selectors without altering values or behavior.Also applies to: 367-367, 594-594
src/patternfly/components/Form/form.scss (1)
463-463: LGTM—non-functional declaration reordering.The
flex-direction: column;property was repositioned within.#{$form}__field-group-header-mainwithout changing its value or effect.src/patternfly/components/FormControl/form-control.scss (1)
248-248: LGTM—non-functional declaration reordering.The
outline-offset: 0;property was moved to appear afterscrollbar-gutter: stablewithin the&.pf-m-textarea > textareaselector, with no change to its value or runtime behavior.src/patternfly/components/Page/page.scss (1)
380-382: Clarify intent: These SCSS changes appear to be non-functional declaration reordering unrelated to the PR objectives.Lines 382 and 487 relocate
flex-direction: column;within their declaration blocks with no semantic impact. The AI summary confirms no behavior change. However, these SCSS modifications seem disconnected from the stated PR goal of reducing Backstop visual-test false positives (Chromium flags, Resemble options, Puppeteer→Playwright migration).Is this reordering intentional (e.g., part of an automated formatter or linting pass) or accidental? If intentional, confirm that similar changes are being applied consistently across the codebase. If unintentional, consider reverting to keep the PR scope focused.
Also applies to: 485-488
src/patternfly/components/ExpandableSection/expandable-section.scss (1)
134-146: Non-functional declaration reordering in.pf-m-truncateblock; clarify scope and automation.Line 141 repositions
overflow: hidden;within the declaration block without affecting CSS output or truncation behavior. This aligns with a broader pattern of declaration reordering across multiple SCSS files in this PR (Page, ModalBox, Tabs, Wizard, etc.), yet these changes are orthogonal to the stated PR objective of reducing Backstop false positives.If this is the result of an automated formatter or linting rule, confirm it's being applied consistently and intentionally across the repository. If not, consider reverting to keep the PR focused and reviewable.
src/patternfly/components/ModalBox/modal-box.scss (1)
162-180: Recurring non-functional declaration reordering; request clarity on PR scope.Lines 165 and 185 reposition
flex-direction: column;declarations within their blocks—a pattern repeated across Page, ExpandableSection, and other components in this PR. The AI summary confirms these are cosmetic-only changes with no impact on rendering or behavior.This systematic reordering suggests either:
- Automated formatting/linting: If so, document the rule and confirm it's applied consistently.
- Accidental bundling: If not intentional, revert to maintain a focused, reviewable PR scope.
Given that the PR's stated objective is reducing Backstop visual-test false positives (via Chromium flags, Resemble options, and Playwright migration), including orthogonal SCSS declaration reorders dilutes the PR's clarity and reviewability.
Also applies to: 182-189
src/patternfly/components/Masthead/masthead.scss (1)
168-172: Non-functional declaration reordering within.pf-m-dockedmodifier.The
flex-direction: columnproperty is repositioned within the._#{$masthead}__contentrule without changing the final computed styles. CSS declaration order within a rule doesn't affect specificity or cascade behavior.src/patternfly/components/Label/label-group.scss (1)
158-176: Non-functional declaration reordering in.textarearule.The
border: 0property is repositioned after-ms-overflow-style: nonewithout affecting the final styles. Both declarations remain effective; order is cosmetic only.src/patternfly/components/Drawer/drawer.scss (1)
334-341: Non-functional declaration reordering in flex container rule.The
flex-direction: columnproperty is repositioned afterflex-shrink: 0within the combined rule for drawer content/panel selectors. Declaration order is cosmetic; all three flex properties (display,flex-shrink,flex-direction) apply identically regardless of sequence.src/patternfly/sass-utilities/mixins.scss (1)
179-186: Verify that this SCSS declaration reordering is intentional.Similar to the tabs.scss file, this change reorders the
overflow: hidden;declaration within thepf-v6-line-clampmixin (line 182), moving it to appear before-webkit-box-orient. This is a non-functional change that doesn't affect the mixin's behavior.This reordering appears unrelated to the PR's stated purpose of adjusting Backstop configuration for visual testing. The presence of similar non-functional reorderings across multiple SCSS files suggests these may have been unintentionally included from a formatter or linter run.
| position: relative; | ||
| flex-direction: column; | ||
| flex-grow: 1; | ||
| flex-direction: column; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Get the current PR branch/commit info and understand scope
echo "=== Git status and recent commits ==="
git log --oneline -n 5
echo -e "\n=== Files changed in this PR ==="
git diff --name-only HEAD~1..HEAD | head -30
echo -e "\n=== Count SCSS files in diff ==="
git diff --name-only HEAD~1..HEAD | grep -c '\.scss$' || echo "0"
echo -e "\n=== All SCSS files changed ==="
git diff --name-only HEAD~1..HEAD | grep '\.scss$'Repository: patternfly/patternfly
Length of output: 834
🏁 Script executed:
# Search for Backstop configuration files and changes
echo "=== Looking for Backstop-related files ==="
find . -type f \( -name "*backstop*" -o -name ".backstoprc*" \) 2>/dev/null | head -20
echo -e "\n=== Checking for Backstop in package.json ==="
grep -i backstop package.json 2>/dev/null || echo "Not found in root package.json"
echo -e "\n=== Checking git diff for Backstop-related changes ==="
git diff HEAD~1..HEAD | grep -i backstop | head -20Repository: patternfly/patternfly
Length of output: 929
🏁 Script executed:
# Examine the actual diff for tabs.scss around lines 388 and 580
echo "=== Examining tabs.scss full diff ==="
git diff HEAD~1..HEAD -- src/patternfly/components/Tabs/tabs.scss | head -100Repository: patternfly/patternfly
Length of output: 154
🏁 Script executed:
# Check git status
echo "=== Git status ==="
git status
echo -e "\n=== Git log detail ==="
git log --oneline -n 10
echo -e "\n=== Check current branch ==="
git branch -aRepository: patternfly/patternfly
Length of output: 351
🏁 Script executed:
# Examine tabs.scss file directly
echo "=== Checking tabs.scss exists ==="
ls -la src/patternfly/components/Tabs/tabs.scss
echo -e "\n=== Lines around 388 in tabs.scss ==="
sed -n '380,395p' src/patternfly/components/Tabs/tabs.scss
echo -e "\n=== Lines around 580 in tabs.scss ==="
sed -n '570,590p' src/patternfly/components/Tabs/tabs.scssRepository: patternfly/patternfly
Length of output: 1795
🏁 Script executed:
# Search for flex-direction and scroll-snap-align in tabs.scss
echo "=== Occurrences of flex-direction in tabs.scss ==="
grep -n "flex-direction" src/patternfly/components/Tabs/tabs.scss
echo -e "\n=== Occurrences of scroll-snap-align in tabs.scss ==="
grep -n "scroll-snap-align" src/patternfly/components/Tabs/tabs.scssRepository: patternfly/patternfly
Length of output: 301
🏁 Script executed:
# Try to compare with main branch to understand the actual changes
echo "=== Comparing current HEAD with main branch ==="
git diff main -- src/patternfly/components/Tabs/tabs.scss | head -150
echo -e "\n=== SCSS files changed between main and current HEAD ==="
git diff --name-only main | grep '\.scss$' | wc -l
git diff --name-only main | grep '\.scss$'Repository: patternfly/patternfly
Length of output: 1984
🏁 Script executed:
# Check if there are actual changes in backstop files
echo "=== Backstop file changes ==="
git diff main -- backstop.config.js backstop.js | head -100Repository: patternfly/patternfly
Length of output: 2687
Intentional stylelint declaration reordering included in this commit.
The CSS declaration reorderings at lines 388 and 580 are confirmed across 13 SCSS files in this commit (flex-direction and scroll-snap-align properties reordered). The commit message "chore: stylelint fixes" indicates these are intentional formatter output.
However, mixing non-functional formatting changes with substantial functional changes (Backstop engine switch to Playwright, new Chromium flags, Resemble configuration, scenario defaults restructuring) makes the PR harder to review. Consider separating pure formatting/linting fixes from functional configuration changes in future PRs—it improves clarity and simplifies review/rollback.
|
Re-ran a full job from main with the latest changes, got the same number of pass/fails - though I didn't accept the previously run screenshots, so I accepted and ran a few one-off's to validate and got 0 failures. Those were with |
|
🎉 This PR is included in version 6.5.0-prerelease.34 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This adjusts and adds some options to backstop to reduce the number of false positives.
Some of these might not be necessary - we could try putting the delay back to 100ms for example.
Details of some of the changes according to Claude:
--force-device-scale-factor=1--font-render-hinting=none--disable-font-subpixel-positioning--disable-gpuresembleOutputOptions:errorType: 'movementDifferenceIntensity'ignoreAntialiasing: trueMost importantSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.