-
Notifications
You must be signed in to change notification settings - Fork 4.9k
feat: support sharding in HTML Reporter #19691
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: support sharding in HTML Reporter #19691
Conversation
|
@microsoft-github-policy-service agree |
7bf0899 to
5364c0a
Compare
pavelfeldman
left a comment
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.
Looks great! Sorry for the late review, just returned from the break.
| } | ||
| this._json = mergeReports(reports); | ||
| } else { | ||
| const zipReader = new zipjs.ZipReader(new zipjs.HttpReader('/report/report.zip'), { useWebWorkers: false }) as zip.ZipReader; |
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.
Having it all in one file is a strong requirement, let's keep it as is. Loading shards from http(s) is Ok though.
| <body> | ||
| <div id='root'></div> | ||
| <script type='module' src='/src/index.tsx'></script> | ||
| <dialog id="fallback-error"> |
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.
Let's remove this: we should be able to open it from file:// - it was a strong requirement from the community.
| const reports: HTMLReport[] = []; | ||
| for (let index = 1; index <= metadata.shard.total; index += 1) { | ||
| try { | ||
| const zipReader = new zipjs.ZipReader(new zipjs.HttpReader(`/report/report-${index}.zip`), { useWebWorkers: false }) as zip.ZipReader; |
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.
Let's keep those files siblings in order to be less intrusive, hard-code less names. So it'll be
new zipjs.HttpReader(`report-${index}.zip`)| const currentJson = await this.entry(`report-${index}.json`) as HTMLReport; | ||
| reports.push(currentJson); | ||
| } catch (error) { | ||
| // Ignore not found error for viewing individual shard report. |
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.
Let's make sure these errors end up in devtools so that it was clear what is going on.
| for (let index = 1; index <= metadata.shard.total; index += 1) { | ||
| try { | ||
| const zipReader = new zipjs.ZipReader(new zipjs.HttpReader(`/report/report-${index}.zip`), { useWebWorkers: false }) as zip.ZipReader; | ||
| for (const entry of await zipReader.getEntries()) |
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.
Looks like you could benefit from fetching them all concurrently instead.
| // Inline metadata. | ||
| const indexFile = path.join(this._reportFolder, 'index.html'); | ||
| fs.appendFileSync(indexFile, '<script>\nwindow.playwrightReportBase64 = "data:application/zip;base64,'); | ||
| fs.appendFileSync(indexFile, `<script>\nwindow.playwrightMetadata = ${JSON.stringify(metadata)}\n</script>`); |
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.
We would need both, the new data and the base64 placeholder.
|
@kevin940726 Heads up: I will pick this up, create a new PR and attribute existing work in the description. This way it will be easier to manage and land the feature. |
|
@dgozman Sure! I'll be away until next week anyway (LNY), so feel free to do anything with this PR :). Also thank you @pavelfeldman for the review! |
This implementation is based on the [original PR](#19691) by @kevin940726. It makes the reporter produce single file when there is no sharding and multiple out-of-line report-x-of-y.zip reports which are automatically merged together when put in one folder. References #10437 Co-authored-by: Kai Hao <[email protected]>
|
Looks like got implemented in #20700. Closing for now then and thanks for the contribution! |
Close #10437.
This feature aims to bring official support to combine sharded reports in the HTML reporter. This is probably not the best solution, but I think it works with minimal changes.
How it works
shardconfig into a global variable along with other metadata for future usage calledwindow.playwrightMetadata.report.zipfile. If there's sharding, we instead write them into areport-{shardIndex}.zipfile.report-{shardIndex}.zipfiles should be overridden and replaced.Comparing to
playwright-merge-html-reportsThe
mergeStatshelper is largely inspired byplaywright-merge-html-reports, but I think this approach has some advantages:Example setup
GitHub Actions with artifacts
WDYT? Once we're happy with the approach we can start writing some docs and tests.