Post process backstop report name - #8067
Closed
srambach wants to merge 3 commits into
Closed
Conversation
Contributor
WalkthroughThis PR removes dynamic title/description logic from Changes
Sequence Diagram(s)sequenceDiagram
participant npm as "npm script"
participant run as "backstop-run.mjs"
participant backstop as "BackstopJS CLI"
participant post as "backstop-post-process.mjs"
participant report as "Report Files"
npm->>run: node scripts/backstop-run.mjs test [--dark] [--desc=...]
run->>backstop: spawn backstop test --config='backstop.js' [--dark]
backstop->>report: generate HTML report (including config.js)
backstop-->>run: exit code
run->>post: node scripts/backstop-post-process.mjs [--desc=...] [--dark]
post->>report: read config.js (JSONP)
post->>post: parse JSONP, update testSuite
post->>report: write updated config.js (JSONP)
post-->>run: exit status (warnings logged if failed)
run-->>npm: propagate BackstopJS exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (1 passed)
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 |
Collaborator
|
Preview: https://pf-pr-8067.surge.sh A11y report: https://pf-pr-8067-a11y.surge.sh |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scripts/backstop-post-process.mjs`:
- Around line 43-46: The JSONP parsing uses a strict regex
content.match(/^report\(([\s\S]*)\);?\s*$/) that fails if config.js has a BOM or
leading whitespace; update the code to either strip a BOM (e.g. content =
content.replace(/^\uFEFF/, '')) and then use a tolerant regex like
content.match(/^\s*report\(([\s\S]*)\);?\s*$/) or directly allow optional
BOM/whitespace in the regex (e.g.
content.match(/^(?:\uFEFF)?\s*report\(([\s\S]*)\);?\s*$/)); change the match
usage accordingly so the variable match still captures the JSON group.
In `@scripts/backstop-run.mjs`:
- Around line 32-69: The runBackstop and runPostProcessor functions currently
build shell command strings (backstopCommand and postProcessCommand) and call
execSync, which permits shell injection and breaks on Windows; update both to
use child_process.spawnSync with argument arrays: for runBackstop call
spawnSync('backstop', [...args, '--config', 'backstop.js'], { stdio:'inherit',
cwd: path.join(__dirname,'..') }) and derive success/exitCode from the returned
.status (and .error when present) instead of catching a thrown error; for
runPostProcessor call spawnSync(process.execPath or 'node',
[path.join(__dirname,'backstop-post-process.mjs'), ...args], { stdio:'inherit',
cwd: path.join(__dirname,'..') }) and similarly propagate success/exitCode —
this removes shell quoting, prevents injection, and fixes Windows quoting
issues.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/backstop-run.mjs`:
- Around line 14-30: Update parseArguments to also handle a standalone '--desc'
token followed by the description as the next argv entry: when iterating args in
parseArguments(), check for arg === '--desc' and if so consume the next element
as customDescription (strip surrounding quotes) and skip pushing both into
backstopArgs; keep existing handling for '--desc=...' intact and ensure you
guard against '--desc' being the last arg (leave customDescription null in that
case and do not push '--desc' to backstopArgs).
Comment on lines
+14
to
+30
| function parseArguments() { | ||
| const args = process.argv.slice(2); | ||
| const backstopArgs = []; | ||
| let customDescription = null; | ||
|
|
||
| for (const arg of args) { | ||
| if (arg.startsWith('--desc=')) { | ||
| // Extract description and remove surrounding quotes | ||
| customDescription = arg.substring(7).replace(/^["']|["']$/g, ''); | ||
| } else { | ||
| // Pass all other arguments to BackstopJS | ||
| backstopArgs.push(arg); | ||
| } | ||
| } | ||
|
|
||
| return { backstopArgs, customDescription }; | ||
| } |
Contributor
There was a problem hiding this comment.
Support --desc passed as a separate arg to avoid leaking it to BackstopJS.
Currently only --desc=... is parsed. If users pass --desc "My report", the flag is forwarded to BackstopJS and the post-processor won’t receive the description.
🛠️ Proposed fix
function parseArguments() {
const args = process.argv.slice(2);
const backstopArgs = [];
let customDescription = null;
- for (const arg of args) {
- if (arg.startsWith('--desc=')) {
- // Extract description and remove surrounding quotes
- customDescription = arg.substring(7).replace(/^["']|["']$/g, '');
- } else {
- // Pass all other arguments to BackstopJS
- backstopArgs.push(arg);
- }
- }
+ for (let i = 0; i < args.length; i++) {
+ const arg = args[i];
+
+ if (arg === '--desc') {
+ const next = args[i + 1];
+ if (!next) {
+ console.error('Error: --desc requires a value');
+ process.exit(1);
+ }
+ customDescription = next.replace(/^["']|["']$/g, '');
+ i++; // consume value
+ continue;
+ }
+
+ if (arg.startsWith('--desc=')) {
+ // Extract description and remove surrounding quotes
+ customDescription = arg.substring(7).replace(/^["']|["']$/g, '');
+ } else {
+ // Pass all other arguments to BackstopJS
+ backstopArgs.push(arg);
+ }
+ }
return { backstopArgs, customDescription };
}🤖 Prompt for AI Agents
In `@scripts/backstop-run.mjs` around lines 14 - 30, Update parseArguments to also
handle a standalone '--desc' token followed by the description as the next argv
entry: when iterating args in parseArguments(), check for arg === '--desc' and
if so consume the next element as customDescription (strip surrounding quotes)
and skip pushing both into backstopArgs; keep existing handling for '--desc=...'
intact and ensure you guard against '--desc' being the last arg (leave
customDescription null in that case and do not push '--desc' to backstopArgs).
Member
Author
|
Closing since it's more complicated than it's worth |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backstop doesn't differentiate the id shown at the top of a report from the initial part of the screen capture filenames.
This PR introduces a wrapper script that changes the name at the top of the report after it's created.
Note: in testing, it works well but (probably because the html report is opened in the browser immediately after creation) you need to reload the page to see the changed name.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.