tests(full-page-screenshot): add node verification and debug tool #15324
Conversation
connorjclark
changed the title
connorjclark
changed the title
| import * as puppeteer from 'puppeteer-core'; | ||
| import {getChromePath} from 'chrome-launcher'; | ||
|
|
||
| import * as LH from '../../../../types/lh.js'; |
There was a problem hiding this comment.
This shouldn't be necessary for a test file
There was a problem hiding this comment.
Ah this test is excluded in the tsconfig.json, so the normal LH global isn't available. I guess this is fine for now.
| @@ -202,4 +208,338 @@ describe('FullPageScreenshot gatherer', () => { | |||
| } | |||
| ); | |||
| }); | |||
There was a problem hiding this comment.
These tests are good, but IMO should completely replace the old test cases. I understand not doing that to make the diff cleaner in this PR tho.
There was a problem hiding this comment.
But they test different things, why would we remove the existing ones?
There was a problem hiding this comment.
The existing ones are only as good as our ability to simulate how the browser handles DPR, page scale, and CDP emulation together. IMO we are never going to get that right so we should just rely on these new e2e tests (and smoke tests) to get this right.
There was a problem hiding this comment.
Bump, could be a follow-up but I still think the old test cases should be converted to smoke tests or this kind of test
| const row = []; | ||
| debugData.push(row); | ||
|
|
||
| for (let x = node.left; x <= right; x++) { |
There was a problem hiding this comment.
This seems a lot like a re-implementation of existing pixel matchers. Could we leverage something like expect-mocha-image-snapshot to do the image diffs for us? We use something similar in LHCI and I found it very helpful to review image diffs in Github that way.
There was a problem hiding this comment.
I'm doing outline detection here, not just a image snapshot. I think this must be a bespoke implementation.
| @@ -202,4 +208,338 @@ describe('FullPageScreenshot gatherer', () => { | |||
| } | |||
| ); | |||
| }); | |||
There was a problem hiding this comment.
Bump, could be a follow-up but I still think the old test cases should be converted to smoke tests or this kind of test
core/scripts/full-page-screenshot-debug.jsto markup a FPS screenshot with its nodes. Some examples: