CI: Make missing manual testing section a CI failure#33345
Conversation
|
View your CI Pipeline Execution ↗ for commit 55be007
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
f918eb7 to
3816249
Compare
377436b to
502ab96
Compare
b35a3d1 to
9234f79
Compare
📝 WalkthroughWalkthroughTwo files are modified: a pull request template is updated to use Markdown admonition syntax for formatting guidance, and a new validation rule is added to dangerfile.js that enforces the presence and substantive content of a manual testing section in PR bodies, with bypass conditions for certain author roles. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/dangerfile.js (1)
131-143: Replace[^]with[\s\S]for clarity and standard compliance.Static analysis correctly flags
[^](negated empty character class) as problematic. While it works in JavaScript to match any character including newlines,[\s\S]is the conventional and more readable pattern.Apply this diff:
const contentWithoutInitialMessage = manualTestingContent - .replace(/>\s*\[!CAUTION\][^]*?This section is mandatory[^]*?Thanks!/i, '') + .replace(/>\s*\[!CAUTION\][\s\S]*?This section is mandatory[\s\S]*?Thanks!/i, '') .replace( /_This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!_/i, '' ) // FIXME: old syntax added to help debug the script until this PR is merged .trim(); // Check if there's any substantial content (ignoring whitespace and template comments) const contentWithoutComments = contentWithoutInitialMessage - .replace(/<!--[^]*?-->/g, '') // Remove HTML comments + .replace(/<!--[\s\S]*?-->/g, '') // Remove HTML comments .replace(/\s+/g, ''); // Remove all whitespace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/danger-js.yml(0 hunks)scripts/dangerfile.js(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/danger-js.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/dangerfile.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to README.md : Update relevant README files for significant code changes
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to README.md : Update relevant README files for significant code changes
Applied to files:
.github/PULL_REQUEST_TEMPLATE.md
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/{addons,frameworks}/**/README.md : Include code examples in addon/framework documentation for significant changes
Applied to files:
.github/PULL_REQUEST_TEMPLATE.md
🪛 Biome (2.1.2)
scripts/dangerfile.js
[error] 133-133: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 133-133: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 142-142: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (4)
.github/PULL_REQUEST_TEMPLATE.md (1)
30-31: Improved visibility using GitHub admonition syntax.The
> [!CAUTION]block draws more attention to the mandatory section requirement and is properly handled by the corresponding regex indangerfile.js.scripts/dangerfile.js (3)
101-110: Bypass logic structure looks good.The author association check correctly excludes bots while allowing maintainers to bypass. The commented
returnwith the TODO is acknowledged in past review comments.
112-129: Section extraction logic is well-structured.The regex correctly identifies the manual testing header and the boundary detection for the next section handles both cases (with/without subsequent sections).
145-155: Validation logic and integration are sound.The empty content check correctly identifies PRs missing substantive manual testing instructions. The integration follows the established pattern alongside
checkRequiredLabelsandcheckPrTitle.
9234f79 to
f56d65e
Compare
f56d65e to
55be007
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/dangerfile.js (1)
132-145: Content validation logic is sound; static analysis warnings are false positives.The validation correctly strips template boilerplate and checks for substantive content.
Regarding the static analysis warnings: The
[^]pattern in JavaScript regex is a standard idiom that matches any character including newlines (equivalent to[\s\S]or.with thesflag). The Biome linter incorrectly interprets this as a negated empty character class. These warnings can be safely ignored.Optional consideration: The regex on line 133 is tightly coupled to the exact wording in the PR template. If the admonition message changes, this validation will break. Consider whether a more flexible pattern (e.g., matching any content within
> [!CAUTION]blocks) would be more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)scripts/dangerfile.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/dangerfile.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to README.md : Update relevant README files for significant code changes
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Ensure all builds and tests pass before submitting pull requests
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to README.md : Update relevant README files for significant code changes
Applied to files:
.github/PULL_REQUEST_TEMPLATE.md
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/{addons,frameworks}/**/README.md : Include code examples in addon/framework documentation for significant changes
Applied to files:
.github/PULL_REQUEST_TEMPLATE.md
🪛 Biome (2.1.2)
scripts/dangerfile.js
[error] 133-133: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 133-133: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 138-138: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
⏰ 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). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
.github/PULL_REQUEST_TEMPLATE.md (1)
30-31: LGTM! Good use of admonition syntax for visibility.The Markdown admonition block makes the mandatory testing guidance more prominent, which aligns well with the CI enforcement being added in dangerfile.js.
scripts/dangerfile.js (4)
113-119: LGTM! Clear validation with helpful error message.The section existence check correctly identifies the manual testing header and provides actionable guidance when missing.
122-129: LGTM! Robust content extraction.The extraction logic correctly handles both mid-document sections and end-of-document cases, with an appropriate regex pattern to identify the next markdown section.
151-151: LGTM! Properly integrated into the validation flow.The function is correctly invoked alongside other PR validation checks when
prLogConfigis available.
104-109: No changes needed. The bot detection property is correct—Danger.js confirms thetypeproperty on GitHub user objects accepts the value "Bot" to identify bot accounts. The bypass logic correctly excludes bots from the OWNER/MEMBER privilege bypass.
What I did
A little experiment because I keep having to tell LLMs disguised as humans to respect our PR template. I'm hoping causing an explicit CI failure will cause more agents to output repro steps we need to speed up reviews.
This is tested via child PRs targeting this branch.
Checklist for Contributors
Testing
Tested through mock PRs targeting this branch.
Manual testing
See:
Documentation
ø
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.