feat(tracing) Adding groups to trace via pw-api

[Snooz82]

This PR implements #33047 .

It shall be possible to add groups to trace logs so that also users that do not use Playwright-Test, like Playwright Python, Robot Framework, Cucumber etc., can also group their traces.
Similar to test.step visually.

[Snooz82]

@microsoft-github-policy-service agree

[dgozman]

Thank you for the PR!

I agree with your choice of ignoring group()/groupEnd() calls when not tracing or when groupEnd() is unbalanced. That seems like the easiest one for the user.

We would need tests that should go into trace-viewer.spec.ts.
Take a look at testd like should have correct stack trace, should show action source and should open simple trace viewer. In the last one, I'd click on a group to expand/collapse it and check the actions list.

I think this should automatically work with the test runner if you follow my other comments. Adding a test similar to this one will ensure that.

Overall, this looks very promising, thank you!

[dgozman]

Instead of handling the stack manually, we should fall back to the default machinery. That probably means calling _wrapApiCall() and introducing an extra parameter to pass a custom location to it, instead of taking the stack trace. This way you'll get source collection and other goodies for free.

You would also need to opt-out of this._isInternalType check inside to make this particular tracing.group() call public. I think that making isInternal argument be boolean | undefined will allow to differentiate explicit false to opt-into public call from default undefined which aligns with this._isInternalType.

[Snooz82]

Actually i do not really understand what you are talking about here, but can it be that this is all irrelevant when using metadata.location as i described in my comment before?

[Snooz82]

@dgozman
Hi Dmitry,
Thanks for your feedback.
Yes i will of course write some test cases. With the hints you provided, i should be able to do them.

Update: Test written

[Snooz82]

PS: It seems to me that you also have some flaky tests?

Is it possible to give the users that do PRs the right, to re-run failed actions?

[Snooz82]

@pavelfeldman @dgozman
Would be really appreciated if someone could check my questions and give feedback what is missing.
This week i have still some time to do fixes or requested changes.

We would really love to see that in next Playwright Release, so that we could implement it in Robot Framework Browser as well.

Thank you.

[dgozman]

@Snooz82 Sorry, I didn't get to this PR today, but I will tomorrow.

• The Firefox bot will recover if you rebase your PR.
• Re: protocol - yes, I was only talking about the internal client-server protocol, not the public API.
• I still think that you'd need to follow my comment about location and _wrapApiCall, but I will be able to apply and try things myself tomorrow.

[Snooz82]

@dgozman Thanks for the feedback.

• fixed internal protocol to have location directly and not typed, but inline definition
• improved docs (I do not know how to link to test.step from api docs.)

You may review again.
In my testing the metadata.location worked absolutely flawless but if you want to change to _wrapApiCall go ahead.

There is one limitation though.
Traces generated by Playwright Test do not correctly show the groups. They interfere with test.step but also without, they are not correctly handled in the actions tree. If i remove the test.trace and rename the 0-trace.trace etc. so that it is a normal trace, all groups work perfectly. I added therefore a hint to the docs.
Imho this limitation is mostly irrelevant, because Playwright Test users should use test.step anyway and Playwright Python, Java, C# and Robot Framework will not use Playwright Test at all.

Thanks for reviewing!
We are pretty exited about this coming into the core! ❤️

[Snooz82]

Hi @dgozman,

I am sorry that i might be annoying.
But if there is anything that i need to do, i could do it at the weekend.
Otherwise, if you want to take over and improve it to a mergeable state, that is also fine.

Thank you.

[dgozman]

Thank you for your patience, this required a bit more work than most of the PRs.

• I've played with the change, and integrated it with the test runner. Please incorporate the following diff: https://gist.github.com/dgozman/2ccb5bbb012f0aa9716b9aa2174ff719.

• Please also add a test for test runner integration, similar to this one. I'd like it to check the following snippet:

  await page.setContent(`<body><div><span><h1>text</h1></span></div></body>`);
  await test.step('custom step', async () => {
    await page.context().tracing.group('outer group');
    await page.context().tracing.group('inner group');
    await page.locator('body').click();
    await page.context().tracing.groupEnd();
    await page.locator('div').click();
    await page.context().tracing.groupEnd();
    await page.locator('span').click();
  });
  await page.locator('h1').click();

• See also other comments.

Thank you for this PR, I think it will turn out as a nice feature.

[Snooz82]

@dgozman

All requested changes are done.

Thanks

[github-actions]

Test results for "tests 1"

36447 passed, 639 skipped
✔️✔️✔️

Merge workflow run.

[Skn0tt]

Hey folks! Chiming in a bit late here. I'm working on a change in a related area so this is interesting to see.

I'm wondering if it'd be better to call this "steps" instead of "groups"? As you're saying, this is pretty similar to steps in how it works and what it does. So we could reuse the term and make it less confusing to users. We could also make the API similar and use Tracing.step(body: Callback) instead of step and stepEnd.

@Snooz82 What are your thoughts on this? Did you consider calling it "steps"? What term do you prefer, and why?

[Snooz82]

@Snooz82 What are your thoughts on this? Did you consider calling it "steps"? What term do you prefer, and why?

Hi @Skn0tt
my initial idea was to call it tracing.startStep & tracing.stopStep but @pavelfeldman proposed group in the original issue #33047 and i was fine wirh that.
my idea was to be more similar to start, startChunk and stopChunk. therefor the startStep would fit into that. i also think startGroup and stopGroup would make sense.

just step and stepEnd would imho make no sense, because it is neither similar to current methods of tracing, nor would it be same as console api in normal JS.

actually i think now that grouping is a more generic word than step. and i would now prefer:

tracing.startGroup()
tracing.stopGroup()

but to be honest, i do not really care. 😉
whatever you decide i take it.
Why? Because the Robot Framework users will anyway never use that feature it will be automatically activated by the Playwright based library they are using.
therefor i do not have any strong feelings here.

i would just hope that this feature could be finalized sooner than later. 🙂

[Skn0tt]

Sounds good! Seeing the discussion in #33047, it looks like the naming has already been considered well :) Taking back my point then.

[Snooz82]

@dgozman
Hi Dmitry,

may i ask if i can do anything to accelerate this PR?
i think it would be cool if at least Playwright Python would have some time to adapt this feature until the release?

best regards
René

[Skn0tt]

Dmitry is currently out of office, so expect to wait a couple more days.

[dgozman]

This looks mostly good. I am going to merge this and follow up with minor improvements.

[Snooz82]

🥳