-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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(html-reporter): add navigation to test details #31793
feat(html-reporter): add navigation to test details #31793
Conversation
This comment has been minimized.
This comment has been minimized.
Looking at unpleasant layout artifact around 0:15 on the video - back / forward buttons should not jump under cursor. |
Good point! Screen.Recording.2024-07-23.at.12.39.04.mov |
Test results for "tests 1"1 failed 8 flaky28987 passed, 694 skipped Merge workflow run. |
<div key={`test-${test.testId}`} className={'test-file-test test-file-test-outcome-' test.outcome}> | ||
<div className='hbox' style={{ alignItems: 'flex-start' }}> | ||
<div className="hbox"> | ||
<span className="test-file-test-status-icon"> | ||
{statusIcon(test.outcome)} | ||
</span> | ||
<span> | ||
<Link href={`#?testId=${test.testId}`} title={[...test.path, test.title].join(' › ')}> | ||
<Link href={`#?q=${q}&testId=${test.testId}`} title={[...test.path, test.title].join(' › ')}> |
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.
With the URL structure like this I can no longer share a link to a test document (?testId) w/o the query involved. That does not sound right. I'll stop my review here since it is pretty fundamental.
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 am not sure what would stop you from sharing this link. It will obviously include a filter query param but it should still work 🤔
Remembering filters is quite important to determine the next items. Passing it as a query param seems like the most natural way. TBH I don't see any other way besides some hacky solutions or rewriting a lot of the code.
If that's a deal-breaker I will just close the PR @pavelfeldman
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.
Imagine me making a bookmark to a test failure and then re-opening it on the next run. But that next time query does not include the test...
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 mode that would retain the test list, while compressing it to the left and opening a wider side view with the test view would make more sense. It would not need to be URL-addressible and would allow convenient navigation. You would be able to pop out (or press copy link button) for test to get into a URL-addressible mode. Let me know if you want to give it a try!
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.
Imagine me making a bookmark to a test failure and then re-opening it on the next run. But that next time query does not include the test...
Yep, that's actually a really good point. I didn't think of that.
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 mode that would retain the test list, while compressing it to the left and opening a wider side view with the test view would make more sense. It would not need to be URL-addressible and would allow convenient navigation. You would be able to pop out (or press copy link button) for test to get into a URL-addressible mode. Let me know if you want to give it a try!
That might be a good idea 🤔
I may think about it but I am not committing to that as of now.
I'll close this one. Thanks for the reviews @pavelfeldman!
I added navigation to the test details view which allows navigating between individual test cases.
I propose to add left and right arrows next to the title. I think it aligns well with the rest of the report but I'd love to hear your proposals.
As of now filter state is cleared after opening test details. Because of that, it's hard to determine what is the next and previous item. That's why I made sure filters are preserved (passing
q
query param).So with the filter state being preserved if I pick only failed tests I will be iterating over failed tests.
If I pick all tests from one project I will be iterating only over those tests.
Check the demo below:
demo.mov