Skip to content
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

Closed

Conversation

kubajanik
Copy link
Contributor

@kubajanik kubajanik commented Jul 22, 2024

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.

Screenshot 2024-07-21 at 14 00 23



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

This comment has been minimized.

@pavelfeldman
Copy link
Member

Looking at unpleasant layout artifact around 0:15 on the video - back / forward buttons should not jump under cursor.

packages/html-reporter/src/reportView.tsx Outdated Show resolved Hide resolved
packages/html-reporter/src/reportView.tsx Show resolved Hide resolved
packages/html-reporter/src/reportView.tsx Outdated Show resolved Hide resolved
packages/html-reporter/src/reportView.tsx Show resolved Hide resolved
@kubajanik
Copy link
Contributor Author

kubajanik commented Jul 23, 2024

Looking at unpleasant layout artifact around 0:15 on the video - back / forward buttons should not jump under cursor.

Good point!
I fixed it - it should be displayed in the same place no matter how long the title is or whether there is a path above.

Screen.Recording.2024-07-23.at.12.39.04.mov

Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [playwright-test] › runner.spec.ts:118:5 › should ignore subprocess creation error because of SIGINT

8 flaky ⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [firefox-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [chromium-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots
⚠️ [webkit-library] › library/browsercontext-fetch-happy-eyeballs.spec.ts:49:3 › get should work on request fixture

28987 passed, 694 skipped
✔️✔️✔️

Merge workflow run.

packages/html-reporter/src/reportView.tsx Show resolved Hide resolved
packages/html-reporter/src/reportView.tsx Show resolved Hide resolved
packages/html-reporter/src/reportView.tsx Show resolved Hide resolved
packages/html-reporter/src/reportView.tsx Show resolved Hide resolved
packages/html-reporter/src/reportView.tsx Show resolved Hide resolved
packages/html-reporter/src/testCaseView.tsx Show resolved Hide resolved
<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(' › ')}>
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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...

Copy link
Member

@pavelfeldman pavelfeldman Jul 26, 2024

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!

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

@kubajanik kubajanik closed this Jul 26, 2024
@kubajanik kubajanik deleted the feat/add-navigation-to-test-details branch July 26, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants