implemented review change requests

Signed-off-by: René <snooz@posteo.de>

packages/playwright-core/src/server/trace/recorder/tracing.ts

@@ -196,6 +196,10 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
     return { traceName: this._state.traceName };
   }
 
+  private _currentGroupId(): string | undefined {
+    return this._state?.groupStack.length ? this._state.groupStack[this._state.groupStack.length - 1] : undefined;
+  }
+
   async group(name: string, location: { file: string, line?: number, column?: number } | undefined, metadata: CallMetadata): Promise<void> {
     if (!this._state)
       return;
@@ -219,8 +223,8 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
       stepId: metadata.stepId,
       stack: stackFrames,
     };
-    if (this._state.groupStack.length)
+    if (this._currentGroupId())
-      event.parentId = this._state.groupStack[this._state.groupStack.length - 1];
+      event.parentId = this._currentGroupId();
     this._state.groupStack.push(event.callId);
     this._appendTraceEvent(event);
   }
@@ -311,7 +315,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
   }
 
   async _closeAllGroups() {
-    while (this._state?.groupStack.length)
+    while (this._currentGroupId())
       await this.groupEnd();
   }
 
@@ -407,10 +411,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
 
   onBeforeCall(sdkObject: SdkObject, metadata: CallMetadata) {
     // IMPORTANT: no awaits before this._appendTraceEvent in this method.
-    const event = createBeforeActionTraceEvent(
+    const event = createBeforeActionTraceEvent(metadata, this._currentGroupId());
-        metadata,
-        this._state?.groupStack.length ? this._state.groupStack[this._state.groupStack.length - 1] : undefined
-    );
     if (!event)
       return Promise.resolve();
     sdkObject.attribution.page?.temporarilyDisableTracingScreencastThrottling();

packages/playwright/src/index.ts

@@ -256,7 +256,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     const artifactsRecorder = new ArtifactsRecorder(playwright, tracing().artifactsDir(), screenshot);
     await artifactsRecorder.willStartTest(testInfo as TestInfoImpl);
 
-    let tracingGroupSteps: TestStepInternal[] = [];
+    const tracingGroupSteps: TestStepInternal[] = [];
     const csiListener: ClientInstrumentationListener = {
       onApiCallBegin: (apiName: string, params: Record<string, any>, frames: StackFrame[], userData: any, out: { stepId?: string }) => {
         const testInfo = currentTestInfo();

tests/library/trace-viewer.spec.ts

@@ -62,15 +62,6 @@ test.beforeAll(async function recordTrace({ browser, browserName, browserType, s
   }
   await doClick();
 
-  await context.tracing.group('High-level Group');
-  await context.tracing.group('First Mid-level Group', { location: { file: `${__dirname}/tracing.spec.ts`, line: 100, column: 10 } });
-  await page.locator('button >> nth=0').click();
-  await context.tracing.groupEnd();
-  await context.tracing.group('Second Mid-level Group', { location: { file: __filename } });
-  await expect(page.getByText('Click')).toBeVisible();
-  await context.tracing.groupEnd();
-  await context.tracing.groupEnd();
-
   await Promise.all([
     page.waitForNavigation(),
     page.waitForResponse(server.PREFIX + '/frames/frame.html'),
@@ -111,65 +102,104 @@ test('should open trace viewer on specific host', async ({ showTraceViewer }, te
   await expect(traceViewer.page).toHaveURL(/127.0.0.1/);
 });
 
-test('should show groups as tree in trace viewer', async ({ showTraceViewer }) => {
+test('should show groups as tree in trace viewer', async ({ runAndTrace, page, context }) => {
-  const traceViewer = await showTraceViewer([traceFile]);
+  const outerGroup = 'Outer Group';
-  await expect(traceViewer.actionTitles).toHaveText([
+  const outerGroupContent = 'locator.clickgetByText(\'Click\')';
-    /browserContext.newPage/,
+  const firstInnerGroup = 'First Inner Group';
-    /page.gotodata:text\/html,<!DOCTYPE html><html>Hello world<\/html>/,
+  const firstInnerGroupContent = 'locator.clicklocator(\'button\').first()';
-    /page.setContent/,
+  const secondInnerGroup = 'Second Inner Group';
-    /expect.toHaveTextlocator\('button'\)/,
+  const secondInnerGroupContent = 'expect.toBeVisiblegetByText(\'Click\')';
-    /expect.toBeHiddengetByTestId\('amazing-btn'\)/,
+  const expandedFailure = 'Expanded Failure';
-    /expect.toBeHiddengetByTestId\(\/amazing-btn-regex\/\)/,
-    /page.evaluate/,
-    /page.evaluate/,
-    /locator.clickgetByText\('Click'\)/,
-    /High-level Group/,
-    /page.waitForNavigation/,
-    /page.waitForResponse/,
-    /page.waitForTimeout/,
-    /page.gotohttp:\/\/localhost:\d+\/frames\/frame.html/,
-    /page.setViewportSize/,
-  ]);
-  await traceViewer.actionsTree.locator('.tree-view-entry:has-text("High-level Group") .codicon-chevron-right').click();
-  await traceViewer.actionsTree.locator('.tree-view-entry:has-text("First Mid-level Group") .codicon-chevron-right').click();
-  await traceViewer.actionsTree.locator('.tree-view-entry:has-text("Second Mid-level Group") .codicon-chevron-right').click();
-  await expect(traceViewer.actionTitles).toHaveText([
-    /browserContext.newPage/,
-    /page.gotodata:text\/html,<!DOCTYPE html><html>Hello world<\/html>/,
-    /page.setContent/,
-    /expect.toHaveTextlocator\('button'\)/,
-    /expect.toBeHiddengetByTestId\('amazing-btn'\)/,
-    /expect.toBeHiddengetByTestId\(\/amazing-btn-regex\/\)/,
-    /page.evaluate/,
-    /page.evaluate/,
-    /locator.clickgetByText\('Click'\)/,
-    /High-level Group/,
-    /First Mid-level Group/,
-    /locator\.clicklocator\('button'\)\.first\(\)/,
-    /Second Mid-level Group/,
-    /expect\.toBeVisiblegetByText\('Click'\)/,
-    /page.waitForNavigation/,
-    /page.waitForResponse/,
-    /page.waitForTimeout/,
-    /page.gotohttp:\/\/localhost:\d+\/frames\/frame.html/,
-    /page.setViewportSize/,
-  ]);
-  await expect(traceViewer.actionsTree.locator('.tree-view-entry:has-text("First Mid-level Group") > .tree-view-indent')).toHaveCount(1);
-  await expect(traceViewer.actionsTree.locator('.tree-view-entry:has-text("Second Mid-level Group") > .tree-view-indent')).toHaveCount(1);
-  await expect(traceViewer.actionsTree.locator('.tree-view-entry:has-text("locator.clicklocator(\'button\').first()") > .tree-view-indent')).toHaveCount(2);
-  await expect(traceViewer.actionsTree.locator('.tree-view-entry:has-text("expect.toBeVisiblegetByText(\'Click\')") > .tree-view-indent')).toHaveCount(2);
 
+  const traceViewer = await test.step('create trace with groups', async () => {
+    return await runAndTrace(async () => {
+      try {
+        await page.goto(`data:text/html,<!DOCTYPE html><html>Hello world</html>`);
+        await page.setContent('<!DOCTYPE html><button>Click</button>');
+        async function doClick() {
+          await page.getByText('Click').click();
+        }
+        await context.tracing.group(outerGroup);  // Outer group
+        await doClick();
+        await context.tracing.group(firstInnerGroup, { location: { file: `${__dirname}/tracing.spec.ts`, line: 100, column: 10 } });
+        await page.locator('button >> nth=0').click();
+        await context.tracing.groupEnd();
+        await context.tracing.group(secondInnerGroup, { location: { file: __filename } });
+        await expect(page.getByText('Click')).toBeVisible();
+        await context.tracing.groupEnd();
+        await context.tracing.groupEnd();
+        await context.tracing.group(expandedFailure);
+        try {
+          await expect(page.getByText('Click')).toBeHidden({ timeout: 1 });
+        } catch (e) {}
+        await context.tracing.groupEnd();
+        await page.evaluate(() => console.log('ungrouped'), null);
+      } catch (e) {}
+    });
+  }, { box: true });
+  const treeViewEntries = traceViewer.actionsTree.locator('.tree-view-entry');
+
+  await test.step('check automatic expansion of groups on failure', async () => {
+    await expect(traceViewer.actionTitles).toHaveText([
+      /page.gotodata:text\/html,<!DOCTYPE html><html>Hello world<\/html>/,
+      /page.setContent/,
+      outerGroup,
+      expandedFailure,
+      /expect.toBeHiddengetByText\('Click'\)/,
+      /page.evaluate/,
+    ]);
+    await expect(traceViewer.actionsTree.locator('.tree-view-entry.selected > .tree-view-indent')).toHaveCount(1);
+    await expect(traceViewer.actionsTree.locator('.tree-view-entry.selected')).toHaveText(/expect.toBeHiddengetByText\('Click'\)/);
+    await treeViewEntries.filter({ hasText: expandedFailure }).locator('.codicon-chevron-down').click();
+  });
+  await test.step('check outer group', async () => {
+    await treeViewEntries.filter({ hasText: outerGroup }).locator('.codicon-chevron-right').click();
+    await expect(traceViewer.actionTitles).toHaveText([
+      /page.gotodata:text\/html,<!DOCTYPE html><html>Hello world<\/html>/,
+      /page.setContent/,
+      outerGroup,
+      outerGroupContent,
+      firstInnerGroup,
+      secondInnerGroup,
+      expandedFailure,
+      /page.evaluate/,
+    ]);
+    await expect(treeViewEntries.filter({ hasText: firstInnerGroup }).locator(' > .tree-view-indent')).toHaveCount(1);
+    await expect(treeViewEntries.filter({ hasText: secondInnerGroup }).locator(' > .tree-view-indent')).toHaveCount(1);
+    await test.step('check automatic location of groups', async () => {
       await traceViewer.showSourceTab();
-  await traceViewer.selectAction('High-level Group');
+      await traceViewer.selectAction(outerGroup);
       await expect(traceViewer.sourceCodeTab.locator('.source-tab-file-name')).toHaveAttribute('title', __filename);
-  await expect(traceViewer.sourceCodeTab.locator('.source-line-running')).toHaveText(/\d+\s+await context.tracing.group\('High-level Group'\);/);
+      await expect(traceViewer.sourceCodeTab.locator('.source-line-running')).toHaveText(/\d+\s+await context.tracing.group\(outerGroup\);  \/\/ Outer group/);
-
+    });
-  await traceViewer.selectAction('First Mid-level Group');
+  });
+  await test.step('check inner groups', async () => {
+    await treeViewEntries.filter({ hasText: firstInnerGroup }).locator('.codicon-chevron-right').click();
+    await treeViewEntries.filter({ hasText: secondInnerGroup }).locator('.codicon-chevron-right').click();
+    await expect(traceViewer.actionTitles).toHaveText([
+      /page.gotodata:text\/html,<!DOCTYPE html><html>Hello world<\/html>/,
+      /page.setContent/,
+      outerGroup,
+      outerGroupContent,
+      firstInnerGroup,
+      firstInnerGroupContent,
+      secondInnerGroup,
+      secondInnerGroupContent,
+      expandedFailure,
+      /page.evaluate/,
+    ]);
+    await expect(treeViewEntries.filter({ hasText: firstInnerGroupContent }).locator(' > .tree-view-indent')).toHaveCount(2);
+    await expect(treeViewEntries.filter({ hasText: secondInnerGroupContent }).locator(' > .tree-view-indent')).toHaveCount(2);
+    await test.step('check location with file, line, column', async () => {
+      await traceViewer.selectAction(firstInnerGroup);
       await expect(traceViewer.sourceCodeTab.locator('.source-tab-file-name')).toHaveAttribute('title', `${__dirname}/tracing.spec.ts`);
-
+    });
-  await traceViewer.selectAction('Second Mid-level Group');
+    await test.step('check location with file', async () => {
+      await traceViewer.selectAction(secondInnerGroup);
       await expect(traceViewer.sourceCodeTab.getByText(/Licensed under the Apache License/)).toBeVisible();
     });
+  });
+});
 
 
 test('should open simple trace viewer', async ({ showTraceViewer }) => {
@@ -184,7 +214,6 @@ test('should open simple trace viewer', async ({ showTraceViewer }) => {
     /page.evaluate/,
     /page.evaluate/,
     /locator.clickgetByText\('Click'\)/,
-    /High-level Group/,
     /page.waitForNavigation/,
     /page.waitForResponse/,
     /page.waitForTimeout/,