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

Playwright: run tests headlessly, take 2. #56712

Merged
merged 4 commits into from
Oct 5, 2021
Merged

Conversation

worldomonation
Copy link
Contributor

Changes proposed in this Pull Request

(Copied over from original PR #56475)

This PR proposes to run Playwright tests headlessly.

By some accounts, Playwright runs faster in headless mode GitHub.

Anecdotally, running a selection of test specs locally with both headful and headless modes show some difference:

Headless:

  [WPCOM] Blocks: CoBlocks: (mobile) @parallel
    ✓ Log in (2541 ms)
    ✓ Start new post (5238 ms)
    ✓ Enter post title (274 ms)
    ✓ Insert Pricing Table block and enter price to left table (2098 ms)
    ✓ Insert Dynamic HR block (996 ms)
    ✓ Insert Hero block and enter heading (1227 ms)
    ✓ Insert Click to Tweet block and enter tweet content (1163 ms)
    ✓ Insert Logos block and set image (2063 ms)
    ✓ Publish and visit post (7967 ms)
    ✓ Confirm Pricing Table block is visible in published post (35 ms)
    ✓ Confirm Dynamic HR block is visible in published post (24 ms)
    ✓ Confirm Hero block is visible in published post (76 ms)
    ✓ Confirm Click to Tweet block is visible in published post (57 ms)
    ✓ Confirm Logos block is visible in published post (29 ms)

Test Suites: 1 passed, 1 total
Tests:       14 passed, 14 total
Snapshots:   0 total
Time:        27.382 s, estimated 32 s

Headful:

  [WPCOM] Blocks: CoBlocks: (mobile) @parallel
    ✓ Log in (2871 ms)
    ✓ Start new post (5304 ms)
    ✓ Enter post title (503 ms)
    ✓ Insert Pricing Table block and enter price to left table (1691 ms)
    ✓ Insert Dynamic HR block (773 ms)
    ✓ Insert Hero block and enter heading (1126 ms)
    ✓ Insert Click to Tweet block and enter tweet content (1072 ms)
    ✓ Insert Logos block and set image (2098 ms)
    ✓ Publish and visit post (7711 ms)
    ✓ Confirm Pricing Table block is visible in published post (44 ms)
    ✓ Confirm Dynamic HR block is visible in published post (24 ms)
    ✓ Confirm Hero block is visible in published post (31 ms)
    ✓ Confirm Click to Tweet block is visible in published post (18 ms)
    ✓ Confirm Logos block is visible in published post (26 ms)

Test Suites: 1 passed, 1 total
Tests:       14 passed, 14 total
Snapshots:   0 total
Time:        31.934 s, estimated 42 s

Testing instructions

  • desktop tests pass
  • mobile tests pass
  • pre-release tests pass

@worldomonation worldomonation requested a review from a team September 30, 2021 18:48
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 30, 2021
@worldomonation worldomonation self-assigned this Sep 30, 2021
@worldomonation worldomonation added the [Pri] High Address as soon as possible after BLOCKER issues label Sep 30, 2021
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@github-actions
Copy link

github-actions bot commented Sep 30, 2021

@scinos
Copy link
Contributor

scinos commented Sep 30, 2021

Interesting, so dropping xvfb-run made the original PR fail? Do we know why?

@worldomonation
Copy link
Contributor Author

worldomonation commented Sep 30, 2021

Interesting, so dropping xvfb-run made the original PR fail? Do we know why?

Yes, it appears that dropping xvfb-run was the cause for failure.

The documentation states that xvfb-run is only required for headed mode, but in practice some comments made by maintainers (eg. this one) appear to imply that it is required even for headless [1]. My experience seem to back up the statement made by the maintainer.

[1] The question was about Electron + headless but I suspect it also applies to browsers.

@scinos
Copy link
Contributor

scinos commented Sep 30, 2021

That's really strange.

Maybe you could add a comment in the build plan explaining why we require xvfb-run even if we run headless? I suspect future maintainers will assume they can remove xvfb-run and will trip over and over with this problem.

@worldomonation
Copy link
Contributor Author

That's really strange.

Maybe you could add a comment in the build plan explaining why we require xvfb-run even if we run headless? I suspect future maintainers will assume they can remove xvfb-run and will trip over and over with this problem.

I plan on exploring a bit more in this PR. I keep getting conflicting results in my research.

I've even expanded my search to just generic Chrome headless and Puppeteer, but still getting mixed results.

@worldomonation
Copy link
Contributor Author

OK, so I dropped xvfb-run - and the tests pass.

😕

@worldomonation
Copy link
Contributor Author

Ran every Playwright e2e task with xvfb-run dropped and it passes.

This PR is now ready for final review.

@worldomonation worldomonation merged commit 708e282 into trunk Oct 5, 2021
@worldomonation worldomonation deleted the pw/headless-1 branch October 5, 2021 17:54
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Pri] High Address as soon as possible after BLOCKER issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants