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

fix(electron): tracing with @playwright/test #31437

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Jun 25, 2024

Fixes #31473

This regressed in e7a11c0#diff-d7c041137e763edae5c4d18bc8ded9a1ac668078e310712341c444ef3aac423b - v1.45.0.

When Electron browser context gets closed, the request object gets disposed. But tracing was never started on the Electron request instance. This throws now:

➜  playwright git:(bugfix/electron-pwt) ✗ npm run ctest --  --trace=on wheel:18

> playwright-internal@1.46.0-next ctest
> playwright test --config=tests/library/playwright.config.ts --project=chromium-* --trace=on wheel:18


Running 1 test using 1 worker
  1) [chromium-page] › page/wheel.spec.ts:18:5 › should work ───────────────────────────────────────

    Error: ENOENT: no such file or directory, open '/Users/maxschmitt/Developer/playwright/test-results/.playwright-artifacts-0/fd9b718e68bb993159f60c54492d3af1.zip'

    attachment #1: trace (application/zip) ─────────────────────────────────────────────────────────
    test-results/wheel-should-work-chromium-page/electron-trace.zip
    Usage:

        npx playwright show-trace test-results/wheel-should-work-chromium-page/electron-trace.zip

    ────────────────────────────────────────────────────────────────────────────────────────────────

  1 failed
    [chromium-page] › page/wheel.spec.ts:18:5 › should work ────────────────────────────────────────

To open last HTML report run:

  npx playwright show-report

➜  playwright git:(bugfix/electron-pwt) ✗ 

In normal PWT setup, this is not a problem, since we already start the tracing before. But since we don't check if the Tracing has been started, it throws now, that we didn't start it before.

I see two solutions:

a) We call didCreateContext in Electron, see footnote

  • Tracing support in Electron
  • Risk which we don't want right now.

b) just ignore the stop call, it wasn't marked with the start symbol.


diff --git a/packages/playwright-core/src/client/electron.ts b/packages/playwright-core/src/client/electron.ts
index a9e90b77f..bb92fdfa5 100644
--- a/packages/playwright-core/src/client/electron.ts
+++ b/packages/playwright-core/src/client/electron.ts
@@ -30,6 +30,7 @@ import { ConsoleMessage } from './consoleMessage';
 import type { Env, WaitForEventOptions, Headers, BrowserContextOptions } from './types';
 import { Waiter } from './waiter';
 import { TargetClosedError } from './errors';
+import { Playwright } from './playwright';
 
 type ElectronOptions = Omit<channels.ElectronLaunchOptions, 'env'|'extraHTTPHeaders'|'recordHar'|'colorScheme'|'acceptDownloads'> & {
   env?: Env,
@@ -42,6 +43,8 @@ type ElectronOptions = Omit<channels.ElectronLaunchOptions, 'env'|'extraHTTPHead
 type ElectronAppType = typeof import('electron');
 
 export class Electron extends ChannelOwner<channels.ElectronChannel> implements api.Electron {
+  _playwright!: Playwright;
+
   static from(electron: channels.ElectronChannel): Electron {
     return (electron as any)._object;
   }
@@ -57,6 +60,7 @@ export class Electron extends ChannelOwner<channels.ElectronChannel> implements
       tracesDir: options.tracesDir,
     };
     const app = ElectronApplication.from((await this._channel.launch(params)).electronApplication);
+    await this._playwright.chromium._didCreateContext(app.context(), params, options, this._logger);
     app._context._setOptions(params, options);
     return app;
   }

@mxschmitt mxschmitt marked this pull request as draft June 25, 2024 14:15

This comment has been minimized.

@mxschmitt mxschmitt marked this pull request as ready for review June 27, 2024 18:56

This comment has been minimized.

This comment has been minimized.

@pavelfeldman
Copy link
Member

I think you are on the right track, but I don't think that is the right place for the fix. Consider a situation where tracing is started, but no trace events are emitted. As the trace file is created lazily, it won't be there when stopping the trace either. So the fix should rather remove the non-existing temporary files from the list before iterating over them in mergeTraceFiles.

You can probably test it using the ttest rather than an installation test btw.

This comment has been minimized.

@mxschmitt mxschmitt added the CQ1 label Jul 1, 2024
@mxschmitt mxschmitt merged commit 9a3e096 into microsoft:main Jul 1, 2024
75 of 107 checks passed
Copy link
Contributor

github-actions bot commented Jul 1, 2024

Test results for "tests 1"

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
⚠️ [chromium-page] › page/page-event-request.spec.ts:138:3 › should report navigation requests and responses handled by service worker with routing
⚠️ [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

28408 passed, 655 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants