Skip to content

test: refactor trace viewer tests to use actual trace viewer #2885

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

Merged
merged 1 commit into from
Jun 11, 2025

Conversation

mxschmitt
Copy link
Member

No description provided.

@mxschmitt mxschmitt force-pushed the use-real-trace-viewer branch from 1d2c129 to a8784e0 Compare June 11, 2025 09:41
@mxschmitt mxschmitt force-pushed the use-real-trace-viewer branch from a8784e0 to 4d23f5c Compare June 11, 2025 10:19
@@ -57,10 +57,12 @@ def headless(pytestconfig: pytest.Config) -> bool:

@pytest.fixture(scope="session")
def launch_arguments(pytestconfig: pytest.Config, headless: bool) -> Dict:
return {
args: Dict = {
Copy link
Member Author

Choose a reason for hiding this comment

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

The background of this change is interesting:

  1. This before returned channel: None -> channel: null
  2. We use this inside npx playwright launch-server --config config.json
  3. Inside Playwright we don't really accept null as a channel option, only undefined, we don't validate it tho and send it to the server
  4. Inside the Server we put the channel value into the context-options and then render it in the Trace Viewer. We manually serialise the context-options before here
  5. This treats null as an object and it ends up as an empty object. We try to render it in React and 'boom' React crashes, since it doesn't allow that.

I'm thinking if this is an edge case in our test or if user-values could also result in such a scenario. I think a null/undefined check makes probably sense there!

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.

This looks good, but I'd like us to improve Trace Viewer, so that our poms are one-liners and we get rid of them entirely.

return self.page.get_by_test_id("stack-trace-list").locator(".list-view-entry")

def select_action(self, title: str, ordinal: int = 0) -> None:
self.page.locator(f'.action-title:has-text("{title}")').nth(ordinal).click()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use something like self.action_titles.filter(has_text=...) instead.

self.page.locator(f'.action-title:has-text("{title}")').nth(ordinal).click()

def select_snapshot(self, name: str) -> None:
self.page.click(f'.snapshot-tab .tabbed-pane-tab-label:has-text("{name}")')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.


@property
def stack_frames(self) -> Locator:
return self.page.get_by_test_id("stack-trace-list").locator(".list-view-entry")
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to update trace viewer, so this becomes get_by_role("list", name="Stack Trace").get_by_role("listitem").

return self.page.frame_locator("iframe.snapshot-visible[name=snapshot]")

def show_source_tab(self) -> None:
self.page.click("text='Source'")
Copy link
Contributor

Choose a reason for hiding this comment

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

Some kind of get_by_role() would be great here!

self.page.click("text='Source'")

def expand_action(self, title: str, ordinal: int = 0) -> None:
self.actions_tree.locator(".tree-view-entry", has_text=title).nth(
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, clicking on a button should have a get_by_role("button").

@mxschmitt mxschmitt merged commit 2c8aa6d into microsoft:main Jun 11, 2025
38 checks passed
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.

2 participants