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
feat: implement CSS Coverage #1714
Conversation
This patch adds two new methods to the `page.coverage` namespace: - `page.coverage.startCSSCoverage()` - to initiate css coverage - `page.coverage.stopCSSCoverage()` - to stop css coverage The coverage format is consistent with the JavaScript coverage.
@@ -2170,22 +2214,6 @@ Identifies what kind of target this is. Can be `"page"`, `"service_worker"`, or | |||
> **NOTE** JavaScript Coverage doesn't include anonymous scripts; however, scripts with sourceURLs are | |||
reported. |
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.
What happens if there is an injected inline style?
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.
The style will be ignored similarly to the anonymous scripts in js coverage. Added a test and a doc note for this.
const coverage = await page.coverage.stopCSSCoverage(); | ||
expect(coverage.length).toBe(0); | ||
}); | ||
}); |
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.
golden file please
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.
Done.
this._stylesheetSources.clear(); | ||
this._eventListeners = [ | ||
helper.addEventListener(this._client, 'CSS.styleSheetAdded', this._onStyleSheet.bind(this)), | ||
helper.addEventListener(this._client, 'Runtime.executionContextsCleared', this._onExecutionContextsCleared.bind(this)), |
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.
It’s a bit odd to listen for executionContextCleared for stylesheets. Might this race against something?
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.
They're all sent from the same process, so no races here
lib/Coverage.js
Outdated
} | ||
|
||
/** | ||
* @return {!Promise<!Array<!{url:string, text:string, ranges:!Array<!{start:number, end:number}>}>>} |
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: can we have this typedef as CoverageData
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.
nit* To force the same format
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.
Done
There's a few open issues regarding coverage. Perhaps some of them can be closed, thanks to this PR? That should raise the visibility and get some early users. |
This patch adds two new methods to the `page.coverage` namespace: - `page.coverage.startCSSCoverage()` - to initiate css coverage - `page.coverage.stopCSSCoverage()` - to stop css coverage The coverage format is consistent with the JavaScript coverage.
This patch adds two new methods to the
page.coverage
namespace:page.coverage.startCSSCoverage()
- to initiate css coveragepage.coverage.stopCSSCoverage()
- to stop css coverageThe coverage format is consistent with the JavaScript coverage.