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
Html report improvements #30094
Html report improvements #30094
Conversation
amankagithub
commented
Mar 25, 2024
•
edited
edited
- Copy annotation text by one click .
- Search test case based on annotations .
This comment has been minimized.
This comment has been minimized.
Please start with filing a feature request, even if you plan to contribute this PR. |
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.
Overall, looks good. There are some comments I'd like you to address and we are missing the tests for the new functionality.
packages/html-reporter/src/filter.ts
Outdated
@@ -114,6 +120,7 @@ export class Filter { | |||
line: String(test.location.line), | |||
column: String(test.location.column), | |||
labels: test.tags.map(tag => tag.toLowerCase()), | |||
annotations: test.annotations.map(a => a.type.toLowerCase() + ':' + a.description?.toLocaleLowerCase()) |
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.
To make sure you only use : once in the filter.
annotations: test.annotations.map(a => a.type.toLowerCase() + ':' + a.description?.toLocaleLowerCase()) | |
annotations: test.annotations.map(a => a.type.toLowerCase() + '=' + a.description?.toLocaleLowerCase()) |
@@ -144,7 +151,14 @@ export class Filter { | |||
if (!matches) | |||
return false; | |||
} | |||
|
|||
if (this.annotations.length) { |
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.
This will return all the tests w/o annotations as matching the search criteria.
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.
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.
Ah, you are right, I missed that this
is a filter here. Still need a test for every new feature though.
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.
I have added the test cases for copy feature . Please review .
return ( | ||
<div className='test-case-annotation'> | ||
<span style={{ fontWeight: 'bold' }}>{type}</span> | ||
<div className='test-case-annotation' onMouseEnter={onHover} onMouseLeave={onLeave}> |
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 looks like you are re-rendering the component that you add listeners to on hover, which might results in unexpected results.
I'd suggest adding a new component next to CopyToClipboard called aaa that would allow copying its content. That way we can use it in more places. I would also recommend that the visibility is implemented using CSS.
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 fully sure what you mean here . Made the required changes based on my what I could understand from your comment .
5b26197
to
3e74564
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
01c472f
to
6463753
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6463753
to
86c5555
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9b3ed73
to
03acbae
Compare
This comment has been minimized.
This comment has been minimized.
packages/html-reporter/src/filter.ts
Outdated
|
||
if (this.annotations.length) { | ||
const matches = this.annotations.every(annotation => | ||
searchValues.annotations.some(_annotation => ( |
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.
style nit: either drop ()
around _annotation.includes(annotation)
or use {}
_annotation.includes(annotation) | ||
))); | ||
if (!matches) | ||
return false; |
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.
Please add a test to tests/playwright-test/reporter-html.spec.ts
that checks that the annotation filter actually works.
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.
Have added the test as you suggested . Please review .
99d56a6
to
3592581
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3592581
to
1a43931
Compare
Test results for "tests 1"27486 passed, 672 skipped Merge workflow run. |
@amankagithub Unfortunately, we found it hard to review and merge this PR. Therefore, it is going slowly, sorry for that! To make this process better, let's follow our general guidelines:
I'll close this PR, since it won't be merged due to the above. |