feat(all): reportMergingTool support single report#2112
feat(all): reportMergingTool support single report#2112frank-mupt wants to merge 1 commit intomainfrom
Conversation
Deploying midscene with
|
| Latest commit: |
dad246a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://236649f0.midscene.pages.dev |
| Branch Preview URL: | https://fix-report-and-misc-improvem.midscene.pages.dev |
quanru
left a comment
There was a problem hiding this comment.
Main concern: the new reportFilePath?: string flow is only implemented in ReportMergingTool, but it is not wired through the report viewer yet.
-
mergeReports()now writes<script type="midscene_web_dump">entries with an empty body whenreportFilePathis missing. However, the viewer still treats every dump script as JSON and eagerly selects the first case on load. If the first case is skipped,dumps[0].get()falls back to a non-GroupedActionDumpobject andsetGroupedDump()still readsdump.executions.length, which can crash the page before the new disabled-click guard has any chance to help. -
Related to that, the
all skippedcase is still broken in the viewer path. The new unit tests only assert that the merged HTML contains the expected attributes, but they do not verify thatapps/reportcan actually load and render those merged reports. -
This also broadens the public type too much. Making
reportFilePathoptional for allReportFileWithAttributesvalues allows invalid states such aspassed + undefined reportFilePath. A narrower model would be safer here, for example a discriminated union for skipped/non-viewable cases instead of weakening the whole API.
Minor point: removing the old <= 1 guard also changes the 0 reports behavior. mergeReports() now creates and returns an empty template-only report when nothing was appended. I think supporting 1 report makes sense, but 0 report should probably still return null or throw explicitly.
e7d8eff to
8007edb
Compare
8007edb to
dad246a
Compare
No description provided.