-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[test] Improve regression test suite debugging #20194
[test] Improve regression test suite debugging #20194
Conversation
if they are in the blacklist they are not needed. The comment should explain the why not the what
test/regressions/index.js
Outdated
function excludeTest(suite, name) { | ||
if (/^docs-premium-themes(.*)/.test(suite)) { | ||
// eslint-disable-next-line no-console | ||
console.log('ingoring premium themes pages'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, I know this should return the reason and logging is a different concern but it's friday :)
Details of bundle changes.Comparing: 0b25383...6584a70 Details of page changes
|
Well it works if you include the bundle locally but vrtest does not display any console calls. |
Ok I honestly don't want to deal with |
'docs-getting-started-templates-album/Album.png', // Flaky image loading | ||
'docs-getting-started-templates-blog', // Not needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Not needed
Maybe we should rename it // Nothing to test
? Basically, these pages are supposed to yield close to no value or a low ROI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there nothing to test? They're not empty as far as I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I imagine the random images from unsplash make it flaky. So the "label" isn't correct. Happy to answer this question for any other test case if you have a doubt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to add any descriptive label you have. The current ones I removed where classic "comments for comments sake"-noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cool, let's add more specific comments. They should add information. "No needed" is equivalent to having the test inside the blacklist array: noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. That helps a lot. Is this good to merge in your opinion? Would be nice to get this in before #20187
vrtest ignores errors in index This reverts commit 3c86385.
ec2b298
to
766b0ae
Compare
Merge after it is rebase with #20195 |
Don't need to rebase since we don't run |
@@ -42,18 +42,20 @@ const blacklist = [ | |||
'docs-components-badges/BadgeAlignment.png', // Redux isolation | |||
'docs-components-badges/BadgeVisibility.png', // Needs interaction | |||
'docs-components-breadcrumbs/ActiveLastBreadcrumb.png', // Redundant | |||
'docs-components-buttons/ButtonBases.png', // Flaky image loading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops
The meat of this PR: b68aedd
The rest is refactoring and debugging features.
Makes the log more verbose but helps debugging which tests are run and which are excluded. It also warns if patterns are unused. This revealed: