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(trace viewer): Extending existing NetworkTab view. #5009

Merged
merged 21 commits into from Jan 26, 2021

Conversation

domderen
Copy link
Contributor

@domderen domderen commented Jan 14, 2021

Currently the network tab contains a limited amount of information on the resources that were loaded in the browser. This change proposes extending the details displayed for each resource, to include:

  • HTTP method,
  • Status
  • Full url,
  • Easily visible response content type,
  • Request headers,
  • Request & response bodies.

Such level of information could help quickly understand what happened in the application, when it was communicating with backend services. This can help debug tests quicker to figure out why they are failing.

What do you think about such extension?

Zrzut ekranu 2021-01-14 o 15 31 33

Zrzut ekranu 2021-01-14 o 19 58 58

Zrzut ekranu 2021-01-14 o 02 53 39

Zrzut ekranu 2021-01-14 o 15 31 59

domderen and others added 8 commits January 14, 2021 02:51
Currently the network tab contains a limited amount of information on the resources that were loaded in the browser. This change proposes extending the details displayed for each resource, to include:

- HTTP method,
- Full url,
- Easily visible response content type,
- Request headers,
- Request & response bodies.

Such level of information could help quickly understand what happened in the application, when it was communicating with backend services. This can help debug tests quicker to figure out why they are failing.

This implementation still needs some clean up & tests improvement, but I wanted to propose such changes and gather your feedback before going too far.

What do you think about such extension?
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a wonderful PR! Sorry for being slow, somehow I missed it when going over PRs. I've added a few comments, but overall looks great.

src/trace/snapshotter.ts Outdated Show resolved Hide resolved
src/cli/traceViewer/web/ui/networkResourceDetails.css Outdated Show resolved Hide resolved
src/cli/traceViewer/web/ui/networkResourceDetails.tsx Outdated Show resolved Hide resolved
src/cli/traceViewer/web/ui/networkTab.tsx Outdated Show resolved Hide resolved
src/cli/traceViewer/web/ui/networkResourceDetails.tsx Outdated Show resolved Hide resolved
src/cli/traceViewer/web/ui/networkResourceDetails.css Outdated Show resolved Hide resolved
@domderen
Copy link
Contributor Author

Hey @dgozman thanks for taking a look at this PR, I think all the changes you pointed out should be fixed now :)

@domderen
Copy link
Contributor Author

domderen commented Jan 24, 2021

One more thing, I was looking at the failing checks, and most of them seems unrelated, but one of them peeked my interest, as it is related to the test I added here: https://github.com/microsoft/playwright/pull/5009/checks?check_run_id=1757111083

It looks like in this particular case, the trace file does not contain the NetworkResourceTraceEvent for /file.json request. I saw that issue before when I was adding those changes, but they went away. I thought it was related to the way the test was set up, but now I'm starting to wonder if there is potentially a race condition somewhere, that doesn't always wait for network events to resolve, before finishing an action like "click". Do you think this makes sense? Should I create a bug for it, and try to look for it?

It is not really related to this PR, so it shouldn't be a blocker, but if that is the case, it might be frustrating for users to miss those network events sometimes. Especially if they are expected on the last action in the script, this network request might not be captured at all in the trace file, like in this failed test run.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few minor comments and this is good to go.

test/trace.spec.ts Show resolved Hide resolved
<div className='network-request-headers'>{resource.requestHeaders.map(pair => `${pair.name}: ${pair.value}`).join('\n')}</div>
<h4>Response Headers</h4>
<div className='network-request-headers'>{resource.responseHeaders.map(pair => `${pair.name}: ${pair.value}`).join('\n')}</div>
{resource.requestSha1 !== 'none' ? <h3>Request Body</h3> : ''}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is h4, but this one is h3 - looks strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, should be fixed now.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@dgozman dgozman merged commit a3af082 into microsoft:master Jan 26, 2021
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