Conversation
|
nice this is a great step, should we expand this for all latencies that weren't realistic? |
|
I can't find the issue now, but we also talked about something like this for when the run was a total disaster...e.g. navStart well after FMP in the trace |
|
@patrickhulce something different than lighthouse/lighthouse-core/audits/load-fast-enough-for-pwa.js Lines 113 to 121 in 1acd240 @brendankenny let's think warnings for overall disasters in another PR. |
brendankenny
left a comment
There was a problem hiding this comment.
Do we need something in the JSON report as well (a top-level warning or whatever)?
|
|
||
| it('renders a warning when using headless chrome', () => { | ||
| const container = renderer._dom._document.body; | ||
| const reportJson = Object.assign(sampleResults, { |
There was a problem hiding this comment.
probably want const reportJson = Object.assign({}, sampleResults, {...}); so sampleResults isn't modified
| const reportJson = Object.assign(sampleResults, { | ||
| userAgent: 'HeadlessChrome/62.0.3180.0 Safari/537.36' | ||
| }); | ||
| const output = renderer.renderReport(reportJson, container); |
There was a problem hiding this comment.
this is changing the contents of the container, which is going to affect other tests in this file since setup/teardown is only done once. Should before/after above actually be beforeEach/afterEach?
| // Render a warning that perf numbers may be off when using headless chrome. | ||
| if (/HeadlessChrome/.test(report.userAgent)) { | ||
| const warning = this._dom.cloneTemplate('#tmpl-lh-headless-warning', this._templateContext); | ||
| reportSection.insertBefore(warning, categories); |
There was a problem hiding this comment.
can this move above categories (and use appendChild) so that it's in the code in the same order it will appear in the report?
There was a problem hiding this comment.
can we add a warning-container to the report DOM permanently, and then this can easily appendChild reliably?
|
@ebidel yeah I meant including that warning with yours as well. Having one place to call it out more prominently in addition to the current spot might prevent some of the WPT discrepancy frustration cases. |
nahhh.. no need. |
|
Blocked by #1512 |
Fixes #2675.
Chose to put this at the top of the report instead of just the perf section for a couple of reasons:
reportJSONthrough a bunch of functions in report-renderer.Should running the module also throw a warning?