report: fix compat for older lighthouse reports #14617
Conversation
connorjclark
force-pushed
the
report-compat
branch
from
5d3f472 to
9cc515b
Compare
| @@ -154,6 +154,10 @@ class Util { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // TODO: convert printf-style displayValue. | |||
There was a problem hiding this comment.
Is this TODO for this PR or later?
There was a problem hiding this comment.
It's an optional, p100 thing :P just something I noticed and wanted to document in the relevant place.
| const waitForAck = viewerPage.evaluate(() => | ||
| new Promise(resolve => | ||
| document.addEventListener('lh-file-upload-test-ack', resolve, {once: true}))); | ||
| await fileInput.uploadFile(`${LH_ROOT}/report/test-assets/${testFilename}`); |
There was a problem hiding this comment.
I don't think this promise structure guarantees that the event listener is installed by the time we call fileInput.uploadFile.
There was a problem hiding this comment.
Protocol commands are executed in sequence, so why wouldn't this be alright? the callback inside the promise is a microtask, but I think even then it runs on the main thread and so would any commands called by fileInput.upload so should be all good
There was a problem hiding this comment.
Protocol commands are executed in sequence
Interesting, did not know this. This is probably fine then.
Fixes #14549
Also added some tests to ensure things are working as far back as v3. The viewer had the best setup for this (it using puppeteer), so I wrote the tests there.