-
Notifications
You must be signed in to change notification settings - Fork 9.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
report: show metric descriptions by default when errors exist #9054
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -239,4 +239,19 @@ describe('ReportUIFeatures', () => { | |
assert.ok(container.querySelector('.score100'), 'has fireworks treatment'); | ||
}); | ||
}); | ||
|
||
describe('metric descriptions', () => { | ||
it('with no errors, hide by default', () => { | ||
const lhr = JSON.parse(JSON.stringify(sampleResults)); | ||
const container = render(lhr); | ||
assert.ok(!container.querySelector('.lh-metrics-toggle__input').checked); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these assertions seem questionable.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as in, I'm very explicitly testing against the if statement. idk seems odd There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it feels like we really just want to assert that the errors are visible, do we do any gCS in these UI tests yet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gCS? I first tried to do something like "are the description elements visible", but I don't think we can do that b/c jsdom doesn't try to surface stuff like that. as in, there is no rendering/css engine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought styles directly applied to elements still resolved though? I could be wrong I just thought there was no cascading. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so I was actually thinking of layout (from reading this). Tried a bit more, but still no dice: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LOL I forgot we shim getComputedStyles in this test. I thought it was provided by jsdom. anyways, idk how to do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
}); | ||
|
||
it('with error, show by default', () => { | ||
const lhr = JSON.parse(JSON.stringify(sampleResults)); | ||
lhr.audits['first-contentful-paint'].errorMessage = 'Error.'; | ||
const container = render(lhr); | ||
assert.ok(container.querySelector('.lh-metrics-toggle__input').checked); | ||
}); | ||
}); | ||
}); |
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.
even better :)