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

Allow for Instanbul coverage with Playwright #2779

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rokob
Copy link

@rokob rokob commented Aug 9, 2024

This makes the PlaywrightLauncherPage function similarly to the ChromeLauncher where we attempt to load the coverage from the browser and return that if availble during the stopSession method. Currently the Playwright launcher breaks early if the native instrumentation is not enabled.

We changed to using the Playwright launcher and were suprised that our coverage stopped working even though we are using the babel istanbul plugin which worked when we were using the ChromeLauncher.

See https://github.com/modernweb-dev/web/blob/master/packages/test-runner-chrome/src/ChromeLauncherPage.ts#L93-L132 for the equivalent code.

Copy link

changeset-bot bot commented Aug 9, 2024

🦋 Changeset detected

Latest commit: 8484bd3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@web/test-runner-playwright Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

This makes the PlaywrightLauncherPage function similarly to the
ChromeLauncher where we attempt to load the coverage from the browser
and return that if availble during the stopSession method. Currently the
Playwright launcher breaks early if the native instrumentation is not
enabled.

We changed to using the Playwright launcher and were suprised that our
coverage stopped working even though we are using the babel istanbul
plugin which worked when we were using the ChromeLauncher.
Copy link
Member

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Is there testing that can confirm this functionality?

@rokob
Copy link
Author

rokob commented Aug 15, 2024

Without this change the newly added test fails with:

  71 passing (47s)
  4 pending
  4 failing

  1) test-runner-playwright chromium
       basic
         coverage
           passes coverage test:
     AssertionError: expected undefined to equal 'something'
      at Context.<anonymous> (/Users/andy.weiss/thirdparty/web/integration/test-runner/tests/basic/runBasicTest.ts:36:43)
      at processImmediate (node:internal/timers:476:21)

  2) test-runner-playwright webkit
       basic
         coverage
           passes coverage test:
     AssertionError: expected undefined to equal 'something'
      at Context.<anonymous> (/Users/andy.weiss/thirdparty/web/integration/test-runner/tests/basic/runBasicTest.ts:36:43)
      at processImmediate (node:internal/timers:476:21)

  3) test-runner-playwright firefox
       basic
         coverage
           passes coverage test:
     AssertionError: expected undefined to equal 'something'
      at Context.<anonymous> (/Users/andy.weiss/thirdparty/web/integration/test-runner/tests/basic/runBasicTest.ts:36:43)
      at processImmediate (node:internal/timers:476:21)

  4) test-runner-playwright all
       basic
         coverage
           passes coverage test:
     AssertionError: expected undefined to equal 'something'
      at Context.<anonymous> (/Users/andy.weiss/thirdparty/web/integration/test-runner/tests/basic/runBasicTest.ts:36:43)
      at processImmediate (node:internal/timers:476:21)

and all tests pass locally for me with this change.

@Westbrook
Copy link
Member

Awesome. I’ll take a closer look at these tests and assuming CI is also happy we’ll look to get this merged, soon! 🙇‍♂️

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