Run playwright in CI - Githubissues

bahrmichael commented 6 months ago

I haven't been able to make progress on this today. Maybe we need to go a level more abstract than using the playwright bin. Most of the time today went into researching if someone else on the internet has used playwright with bazel (I'm surprised by how little info there is), testing some more abstract approaches like genrules, and refreshing my bazel knowledge.

bahrmichael commented 6 months ago

I've made a bit of progress as in "we now run the step to install playwright dependencies, but fail to do so because of system permissions". See https://buildkite.com/sourcegraph/sourcegraph/builds/264578#018e2d1c-b3b9-420a-8e96-64b447e1d663.

I've also spent some time to revisit the current state of nodejs tooling for Bazel (esp. rules_nodejs from aspect).

The problem I'm working on now is that we need to let playwright set up some resources so that it can run its tests. There doesn't seem to be any good resource on running playwright with Bazel. Please let me know if you know any!

Here are some options I see that I'll explore a bit

passing additional parameters to playwright install that may improve CI compatibility (e.g. --with-deps).
hermetic install: https://playwright.dev/docs/browsers#hermetic-install
docker image provided by playwright (need to figure out how to best run tests via a container in Bazel)
- Already used, and probably preferred approach (not sure yet if this is just for testing containers themselves, or for arbitrary tests): https://github.com/GoogleContainerTools/container-structure-test
- Deprecated docker rules that can run a container: https://github.com/bazelbuild/rules_docker
installing dependencies without the playwright install command
changing user permissions to resolve error from the build above (last option because I don't want to interfere with default permissions to much)
don't run playwright tests in Bazel but via GitHub actions

bahrmichael commented 6 months ago

playwright install (with or without extra args) seems blocked, because of permissions.

https://buildkite.com/sourcegraph/sourcegraph/builds/264611#018e2d93-f5a9-40c3-b8b8-b851b9567012

Installing dependencies...
E: setgroups 65534 failed - setgroups (1: Operation not permitted)

bahrmichael commented 6 months ago

No progress on hermetic install either.

With hermetic env var

https://buildkite.com/sourcegraph/sourcegraph/builds/264631#018e2dc5-4475-4a6c-a531-ccd0266d934f

================================================================================
(13:48:31) FAIL: //client/web:playwright_install (see /root/.cache/bazel/_bazel_root/157e7a032b8afdaab0b3a3266b4b5f43/execroot/__main__/bazel-out/k8-fastbuild/testlogs/client/web/playwright_install/test.log)
FAILED: //client/web:playwright_install (Summary)
      /root/.cache/bazel/_bazel_root/157e7a032b8afdaab0b3a3266b4b5f43/execroot/__main__/bazel-out/k8-fastbuild/testlogs/client/web/playwright_install/test.log
      /root/.cache/bazel/_bazel_root/157e7a032b8afdaab0b3a3266b4b5f43/execroot/__main__/bazel-out/k8-fastbuild/testlogs/client/web/playwright_install/test_attempts/attempt_1.log
(13:48:31) INFO: From Testing //client/web:playwright_install:
==================== Test output for //client/web:playwright_install:
Installing dependencies...
E: setgroups 65534 failed - setgroups (1: Operation not permitted)
E: setegid 65534 failed - setegid (22: Invalid argument)
E: seteuid 100 failed - seteuid (22: Invalid argument)

Without `--with-deps`:

https://buildkite.com/sourcegraph/sourcegraph/builds/264634#018e2dcd-f863-44c4-acaa-c4b451d14a7f

==================== Test output for //client/web:playwright_install:
Downloading Chromium 123.0.6312.4 (playwright build v1105) from https://playwright.azureedge.net/builds/chromium/1105/chromium-linux.zip
Error: getaddrinfo EAI_AGAIN playwright.azureedge.net
    at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:internal/dns/promises:95:17) {
  errno: -3001,
  code: 'EAI_AGAIN',
  syscall: 'getaddrinfo',
  hostname: 'playwright.azureedge.net'
}
Downloading Chromium 123.0.6312.4 (playwright build v1105) from https://playwright-akamai.azureedge.net/builds/chromium/1105/chromium-linux.zip
Error: getaddrinfo EAI_AGAIN playwright-akamai.azureedge.net
    at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:internal/dns/promises:95:17) {
  errno: -3001,
  code: 'EAI_AGAIN',
  syscall: 'getaddrinfo',
  hostname: 'playwright-akamai.azureedge.net'
}
Downloading Chromium 123.0.6312.4 (playwright build v1105) from https://playwright-verizon.azureedge.net/builds/chromium/1105/chromium-linux.zip
Error: getaddrinfo EAI_AGAIN playwright-verizon.azureedge.net
    at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:internal/dns/promises:95:17) {
  errno: -3001,
  code: 'EAI_AGAIN',
  syscall: 'getaddrinfo',
  hostname: 'playwright-verizon.azureedge.net'
}
Failed to install browsers
Error: Failed to download Chromium 123.0.6312.4 (playwright build v1105), caused by
Error: Download failure, code=1
    at ChildProcess.<anonymous> (/tmp/bazel-working-directory/__main__/bazel-out/k8-fastbuild/bin/client/web/playwright_install.sh.runfiles/__main__/node_modules/.aspect_rules_js/playwright-core@1.42.1/node_modules/playwright-core/lib/server/registry/browserFetcher.js:91:16)
    at ChildProcess.emit (node:events:514:28)
    at ChildProcess._handle.onexit (node:internal/child_process:294:12)

With alternative but unrecommended approach

Fails locally (if I npx uninstall playwright before) because it fails to load the new dependencies during Bazel runtime. It works fine when just running playwright or the npm scripts.

Error: browserType.launch: Executable doesn't exist at /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/sandbox/darwin-sandbox/9132/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web/playwright_test.sh.runfiles/__main__/node_modules/.aspect_rules_js/playwright-core@1.40.1/node_modules/playwright-core/.local-browsers/chromium-1091/chrome-mac/Chromium.app/Contents/MacOS/Chromium

bahrmichael commented 6 months ago

Related breaking change where playwright stopped downloading browsers automatically (~7 months ago):

https://playwright.dev/docs/release-notes#breaking-changes-playwright-no-longer-downloads-browsers-automatically

bahrmichael commented 6 months ago

Another idea:

instead of npm browser dependencies use http_archive to download a binary into a well-known path and pass that as the browser path env
figure out where the binary from the browser dependencies is and pass that with the browser path env
talk to James about Bazel and maybe running docker containers inside of it

bahrmichael commented 6 months ago

Main difference between puppeteer and playwright: Puppeteer downloads the browser during npm install; playwright has its own CDN and expectations on the folder structure where the apps live. I feel like mixing the two would be a bad idea.

bahrmichael commented 6 months ago

I have made some progress and documented on the PR in the meantime: https://github.com/sourcegraph/sourcegraph/pull/60897#issuecomment-2009967872

I solved the install problems above, and now face a different challenge when integrating playwright with our new sveltekit app.

I'm stuck with a Bazel problem/understanding issues. After demonstrating that we can run playwright in CI (after installing the browser) and test arbitrary websites, I'm now trying to make this setup work with a preview server from the webapp rewrite. The main difference here is that there's no webserver running somewhere, but we also need to start and use the preview webserver.

The current BAZEL.build contains a call to vite to build a production build, which should then be used for the preview server.

https://github.com/sourcegraph/sourcegraph/blob/99d6bbabebb4c4f60472d4b209b9efd339d81480/client/web-sveltekit/BUILD.bazel#L112-L126

The current playwright config however builds and starts the preview server again:

https://github.com/sourcegraph/sourcegraph/blob/99d6bbabebb4c4f60472d4b209b9efd339d81480/client/web-sveltekit/playwright.config.ts#L8

My goal is that we can run playwright locally as well as on CI.

With that in mind I see a couple approaches we could take:

Keep the playwright.config.ts as is, and don't let the build and test targets depend on each other.
Create dedicated build, preview, and test targets in bazel, which depend on one another.
A mix of both.

Here's what my attempts yielded:

Keep playwright.config.ts as is

See https://github.com/sourcegraph/sourcegraph/pull/60897/commits/0697f1a7cb4516e5dc6b631c6bc9f06745d96f9a and https://github.com/sourcegraph/sourcegraph/pull/60897/commits/465c952a760c39b90f3cb136b708e607a6dba966

The build fails, because the vite command is not available.

When running bazel test "//client/web-sveltekit:*" locally this is what I see:

[WebServer] sh: vite: command not found
Error: Process from config.webServer was not able to start. Exit code: 127

My assumption was that it would be available, because npm_link_all_packages should make the installed files available. The version from before my changes also uses vite (":node_modules/vite").

I'm reluctant to moving away from the playwright_test_bin.playwright_test target, because it took me a while to use that in a way that lets me run playwright tests.

A mix of both

Using the existing build target, and introducing a playwright target that omits the build, leads me to file conflicts where I'm not sure what the right way to resolve is (lack of Bazel experience). The package.json file is needed in both targets to (1) run the right build and (2) start the preview server with the same approach that devs would use.

See commit https://github.com/sourcegraph/sourcegraph/pull/60897/commits/055ce8bd513aaede4f9557c81c33b45237dc06c3

When running bazel test "//client/web-sveltekit:*" locally this is what I see:

[WebServer] npm[WebServer]  ERR! code ENOENT
npm ERR! syscall open
npm ERR! path /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/client/web-sveltekit/package.json
npm ERR! errno -2
[WebServer] npm ERR! enoent Could not read package.json: Error: ENOENT: no such file or directory, open '/private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/client/web-sveltekit/package.json'

I have experimented with copy_data_to_bin=False but haven't been able to get it working that way. One problem disappears and others start to pop up which appear to be caused by setting that flag.

Create dedicated build, preview, and test targets

With this approach I assume that I can introduce a new preview target that runs the preview server and keeps it alive for the playwright target that depends on it. Playwright now assumes that a web server is available somewhere.

See commit https://github.com/sourcegraph/sourcegraph/pull/60897/commits/c905a5dbd773ffcc0601fac9a15680adc8b2574e

When running bazel test "//client/web-sveltekit:*" locally this is what I see:

FAIL: //client/web-sveltekit:e2e_test (see /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/testlogs/client/web-sveltekit/e2e_test/test.log)
INFO: From Testing //client/web-sveltekit:e2e_test:
==================== Test output for //client/web-sveltekit:e2e_test:
INFO: aspect_rules_js[js_test]: js_binary TARGET //client/web-sveltekit:e2e_test
INFO: aspect_rules_js[js_test]: js_binary RUNFILES /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles
INFO: aspect_rules_js[js_test]: js_binary EXECROOT /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__
INFO: aspect_rules_js[js_test]: PWD /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/client/web-sveltekit
INFO: aspect_rules_js[js_test]: running /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/client/web-sveltekit/e2e_test_node_bin/node --preserve-symlinks-main -- /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/node_modules/.aspect_rules_js/@playwright+test@1.42.0/node_modules/@playwright/test/cli.js test --config ./playwright.config.ts
Error: Playwright Test did not expect test.beforeEach() to be called here.
Most common reasons include:
- You are calling test.beforeEach() in a configuration file.
- You are calling test.beforeEach() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
  when one of the dependencies in your package.json depends on @playwright/test.

   at ../../../../src/routes/[...repo=reporev]/(validrev)/-/branches/all/page.spec.ts:6

  4 | const url = `/${repoName}/-/branches/all`
  5 |
> 6 | test.beforeEach(async ({ sg }) => {

I've seen this problem before, where multiple dependencies caused this issue. Back then I had both playwright and @playwright/test in my dependencies, and resolved this by switching to playwright-core. As far as I can see that's not the root cause now.

Before I spend more time on resolving this dependency issue, I'd like to get feedback on the approaches, what path is the best to move forward with, and if you have tips on how I could fix some of the issues?

fkling commented 6 months ago

I've seen this problem before, where multiple dependencies caused this issue.

A gut reaction is that we should import a different version of playwright. I see you imported

load("@npm//client/playwright:@playwright/test/package_json.bzl", playwright_test_bin = "bin")

if you want to use the same version as client/web-sveltekit uses it should be

load("@npm//client/web-sveltekit:@playwright/test/package_json.bzl", playwright_test_bin = "bin")

bahrmichael commented 6 months ago

Good catch! I just added the commit https://github.com/sourcegraph/sourcegraph/pull/60897/commits/c905a5dbd773ffcc0601fac9a15680adc8b2574e which I forgot to create yesterday, and fixed the import there. However I'm still getting the same error.

The previous playwright prototype also used the same import syntax, so there should be nothing wrong with it.

load("@npm//client/playwright:@playwright/test/package_json.bzl", playwright_test_bin = "bin")

I have not been able to find any other dependencies that pull in playwright and lead to conflicts. But then I'm not sure how one would debug dependency conflicts in Bazel.

Taking a closer look at the error message I noticed that apparently there's a regular playwright that's being used somewhere (which explains the conflicting versions).

/private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/playwright@1.42.0/node_modules/playwright/lib/common/testType.js:71:13

jamesmcnamara commented 6 months ago

@bahrmichael What we've done in the past for this is just stub out any calls from the test driver to just return static files. So for example we'd intercept all calls to the server URL and statically return the manifest file. Then in turn we'd catch any asset file requests and serve them from the file system.

The benefit is that the test process is greatly simplified; there aren't two parallel communicating processes that need to be brought up, managed and torn down, just a single process (which you already have) which resolves all of its own requests. Particularly in CI this makes life much easier, and the same approach can be run locally.

The drawbacks that I can think of are:

This can be quite surprising behavior. When people see browser tests people assume there is some amount of backend running (However this is already somewhat true if you are mocking out API responses)
If your web server is doing non-trivial routing logic, that must be duplicated or imported into your test and maintained in parallel (However if you're just trying to recreate a vite preview web server this shouldn't apply)

If you like this approach I can work on it tomorrow (not sure how long it will take because I don't know enough about how the svelte app assets bundling / manifest / requests differ from the existing react-app).

If for some reason we need to run vite preview unequivocally, then I'll have to do some more research. What we have today for our "true" e2e tests are server_integration_tests like e2e_test that actually start up a docker image via a bash script, so not the most official or easily supervisable technique

bahrmichael commented 6 months ago

@jamesmcnamara Thank you very much for your input! I took some time to investigate the intercept approach you suggested, and while not able to make a test green while intercepting all calls I feel like this might be the better approach.

You can see my attempt in https://github.com/sourcegraph/sourcegraph/pull/60897/commits/059a1cb9cd00abe7c1364b9460ea9fcc5405e3a8, where I've set up most intercepts except for the indexHtml. I skipped that part because I wanted to get an understanding of polly and the server intercepts before jumping into our custom code and figuring out what needs to be copied/adapated.

If you want to pick up here and make the finishing touches on the index/manifest part, this section might be a good starting point to add new interceptors to: https://github.com/sourcegraph/sourcegraph/pull/60897/commits/059a1cb9cd00abe7c1364b9460ea9fcc5405e3a8#diff-fe440130504bd56a8be70fd3da04f0d03ac583cc4fd8134fa96f64b09120fd26R61-R67

I've seen the _app/version.json request running through my interceptor, so the setup seems to be working in general.

When we run pnpm run build we get a build folder that holds the assets we want to test against, and I found the manifest.json in the .svelte-kit folder (client/web-sveltekit/.svelte-kit/output/client/.vite/manifest.json). I'm not confident enough about manifest files to say that this is the right one.

jamesmcnamara commented 6 months ago

@bahrmichael I added a commit that makes all the tests green :) I removed Polly and just used playwright's native network mocking. As far as the manifest, we're in luck that vite actually produces a valid index.html file with the appropriately generated entrypoints, so we don't have to worry about the manifest at all and can just resolve the requests straight to index.html.

bahrmichael commented 6 months ago

@jamesmcnamara Thank you very much! Happy that it turned out a bit easier, that should make it more maintainable in the long term :) I can run the tests locally and can confirm that it works well.

Where we still need a bit of work is about running it in CI and resolving the issues playwright seems to have with conflicting dependencies (see "Create dedicated build, preview, and test targets" above).

Playwright says for each test that either a test is (indirectly) called from a config file, or that there are version conflicts between playwright dependencies.

Error: Playwright Test did not expect test() to be called here.
Most common reasons include:
- You are calling test() in a configuration file.
- You are calling test() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
  when one of the dependencies in your package.json depends on @playwright/test.

I'm somewhat confident that we're not seeing the config file issue, because I trimmed down tests to import nothing but @playwright/test and they still failed with the same error. This is also supported by the fact that tests run well without Bazel (hinting that something outside the test file is off).

As for the version conflicts, I have done a bunch of tests. First I went through the pnpm lock file and updated dependencies in package files to make sure the same version of playwright is used everywhere. That didn't resolve the issues though. I later tried deliberately using separate versions in the hope that maybe Bazel was confusing multiple equal versions, no success either. Another attempt I did was to copy the setup from my earlier playwright tests, where I was able to run playwright tests which imported @playwright/test.

I threw some more ideas against the wall in https://github.com/sourcegraph/sourcegraph/pull/60897/commits/86021aadf9ecbb93ab60b13f7587e344cf104de3, but got the same error.

The question that confuses me right now: Why did it work in https://github.com/sourcegraph/sourcegraph/commit/cc51271fc07187f0fb106ede88b77bc766058937 but not in https://github.com/sourcegraph/sourcegraph/pull/60897/commits/79da3f412ce4e082f251511bd5909918ea70b2a3?

Do you have an idea of what could cause this error in Bazel and how we could resolve it?

Side question: It looked to me like Bazel did not reset dependencies when I changed a version, and the only approach to make sure it pulls all the latest dependencies was to bazel clean --async and then run the target again. That adds 10-20 minutes of waiting time though. Is there a more efficient way to doing that?

bahrmichael commented 6 months ago

Some more things I tested:

In my previous version with client/playwright, I didn't use chdir on the target. I attempted running the e2e target without chdir in https://github.com/sourcegraph/sourcegraph/commit/0d3a28b9b407804d01b91d8dcdd565d8d05750e7.

bahrmichael commented 6 months ago

New insights!

I think that this issue is not actually caused by multiple versions of playwright, but by a combination of how playwright, bazel and node work. The culprit here is is a conflict between how Playwright loads files, and that Bazel works a lot with symlinks which leads to the same playwright module being loaded multiple times.

I stumbled upon a playwright issue where they had a bug related to lower/upper casing of directories. I added the snippet from the issue to the globals file that Playwright uses during Bazel, and got confirmation that there is indeed a loading conflict.

const s = Symbol.for('__playwright');
if (globalThis[s]) {
  throw new Error(`more than one instance of globals.js was loaded`);
} else {
  Reflect.defineProperty(globalThis, s, { value: true, configurable: true });
}

INFO: aspect_rules_js[js_test]: running /.../bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/client/web-sveltekit/e2e_test_node_bin/node --preserve-symlinks-main -- /.../bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright/cli.js test --config ./playwright.config.ts
Error: more than one instance of globals.js was loaded
    at Object.<anonymous> (/.../bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright/lib/common/globals.js:5:9)

There's also this issue where people are seeing similar problems.

Some options we can explore to resolve this issue:

Figure out what's wrong with Bazel symlinks
Write a patch for playwright (might need a fork)
(Done) Use the nodejs_binary (=> js_test with aspect) approach mentioned here

bahrmichael commented 6 months ago

I tried three variations of the approach to use js_test instead with an entrypoint.

node_modules entry with playwright dependency

js_test(
    name = "e2e_test",
    args = [
        "test",
        "--config ./playwright.config.ts",
    ],
    chdir = package_name(),
    data = [
        ":test_files",
        ":web-sveltekit",
        "//client/web-sveltekit:node_modules/@playwright/test",
        "//client/web-sveltekit:node_modules/playwright",
    ],
    entry_point = "node_modules/playwright/cli.js",
    env = {
        "BAZEL": "1",
        "PW_RUNNER_DEBUG": "true",
    },
    local = True,
    log_level = "info",
    tags = [
        "no-sandbox",
        "requires-network",
    ],
)

Here we're telling js_test to call node_modules/playwright/cli.js. This however leads to a conflict between the data import and the entrypoint:

ERROR: One of the output paths 'bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/node_modules/playwright/cli.js' (belonging to //client/web-sveltekit:e2e_test) and 'bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/node_modules/playwright' (belonging to //client/web-sveltekit:node_modules/playwright) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them

node_modules entrypoint without dependency

js_test(
    name = "e2e_test",
    args = [
        "test",
        "--config ./playwright.config.ts",
    ],
    chdir = package_name(),
    data = [
        ":test_files",
        ":web-sveltekit",
        "//client/web-sveltekit:node_modules/@playwright/test",
        # "//client/web-sveltekit:node_modules/playwright",
    ],
    entry_point = "node_modules/playwright/cli.js",
    env = {
        "BAZEL": "1",
        "PW_RUNNER_DEBUG": "true",
    },
    local = True,
    log_level = "info",
    tags = [
        "no-sandbox",
        "requires-network",
    ],
)

This leads to an error, because Bazel only copies the entrypoint, but not the rest of the module that playwright needs:

Error: Cannot find module './lib/program'
Require stack:
- /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/client/web-sveltekit/node_modules/playwright/cli.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1048:15)

Use a dedicated script to start the playwright process with playwright dependencies in data

Here's the Bazel step:

js_test(
    name = "e2e_test",
    args = [
        "test",
        "--config ./playwright.config.ts",
    ],
    chdir = package_name(),
    data = [
        # ":playwright_install",
        ":test_files",
        ":web-sveltekit",
        "//client/web-sveltekit:node_modules/@playwright/test",
        "//client/web-sveltekit:node_modules/playwright",
    ],
    entry_point = "playwright_runner.js",
    env = {
        "BAZEL": "1",
        "PW_RUNNER_DEBUG": "true",
    },
    local = True,
    log_level = "info",
    tags = [
        "no-sandbox",
        "requires-network",
    ],
)

And here's the playwright runner:

import {spawnSync} from 'child_process';

const result = spawnSync('node node_modules/playwright/cli.js test --config playwright.config.ts', { encoding: 'utf8', shell: '/bin/bash' });
console.error('Execution Result: \n', result.stdout);
throw Error(JSON.stringify(result.stderr))

With this we get the "dependency" error again, so I think this one didn't help with fixing Bazel symlinks.

Error: Playwright Test did not expect test.describe() to be called here.
Most common reasons include:
- You are calling test.describe() in a configuration file.
- You are calling test.describe() in a file that is imported by the configuration file.
- You have two different versions of @playwright/test. This usually happens
  when one of the dependencies in your package.json depends on @playwright/test.

jamesmcnamara commented 6 months ago

@bahrmichael Alright I made some progress on this here. After a lot of digging it seems the problem comes from there actually being two copies of playwright somehow in the bundle. After cutting down a lot of the overlapping dependencies in the main :web-sveltekit target, I got some different errors.

The changes in that commit omits the dep on :web_sveltekit (so it won't work) but it's actually able to run all of our tests, but they just fail when they try to load build/index.html because that is an output of :web_sveltekit. So I feel like somewhere in that target we're pulling in another copy of playwright somehow.

The other issue is that our test fixture calls import.meta.url to construct the path to the schema object which isn't available when this is run in commonjs land. So we'll have to either find a way to pre-compile that code as a module or replace that path with something else.

I'll keep looking at this tomorrow.

bahrmichael commented 6 months ago

@jamesmcnamara Very happy to see your progress! I can confirm that without importing :web-sveltekit into the e2e_test target I get the same errors that you describe.

I tried to dive deeper into this issue, and tried some variations (e.g. using playwright instead of @playwright/test in the build file or using preserve_symlinks_main = False, on the e2e_test target. I also renamed :web-sveltekit to :build locally to test if there are any shadowing issues between the package name and the target. No change in results though.

Another approach I took was to add a console.trace into the globals.js of playwright (aside from the script that causes an error if that fail is loaded twice).

$ head client/web-sveltekit/e2e_test.sh.runfiles/__main__/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright/lib/common/globals.js
"use strict";

const s = Symbol.for('__playwright');
if (globalThis[s]) {
  throw new Error(`more than one instance of globals.js was loaded`);
} else {
  console.trace() // This trace is new
  Reflect.defineProperty(globalThis, s, { value: true, configurable: true });
}

The trace inside the globals.js points to two files on the system, which should be the same based on their symlink:

bazel-out/darwin_arm64-fastbuild/bin/client/web-sveltekit/e2e_test.sh.runfiles/__main__/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright/lib/common/globals.js:7:11

bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright/lib/common/globals.js:7:11

Symlink:

$ cd bazel-out/darwin_arm64-fastbuild
$ ls -al client/web-sveltekit/e2e_test.sh.runfiles/__main__/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright    
lrwxr-xr-x@ 1 michael  wheel  191 Mar 29 17:25 client/web-sveltekit/e2e_test.sh.runfiles/__main__/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright -> /private/var/tmp/_bazel_michael/680fb57cd51801cfe03bf19f9d7a0d3e/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright

After two traces appear, we then see an error from this file:

bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/playwright@1.42.1/node_modules/playwright/lib/common/globals.js:5:9

I find it interesting that two traces can appear before an error is raised. I was expecting only one trace, but then I'm not sure how well node deals with symlinks.

If this task is turning out too large, I'll be happy to recap things into a new issue :)

jamesmcnamara commented 6 months ago

Little update: I've zeroed in on the exact line that is causing the issue (in brief if the test suites are loaded as ESM it's reinitializing playwright in a new module context, but if it loads it as CommonJS, references to playwright are resolved correctly). I'll keep looking at this tomorrow

bahrmichael commented 5 months ago

@jamesmcnamara Just wanted to check in if you've been able to make more progress. Since I'm not able to make progress myself I want to be supportive with whatever approach you think is best :) If the challenge of ESM vs. CommonJS is turning out to difficult, we could also revisit the "true" web server approach. Furthermore, if this is turning out too large of an effort to do on the side, I'll be happy to funnel this into something more official. Let me know what you think is the best path forward at this time.

bahrmichael commented 5 months ago

I'm continuing to track this ticket. My current status is that things have been set up so that we can work with Aspect.

bahrmichael commented 4 months ago

The main work is now done in https://github.com/sourcegraph/sourcegraph/pull/62560

sourcegraph / sourcegraph-public-snapshot

Run playwright in CI #60881

With hermetic env var

Without `--with-deps`:

With alternative but unrecommended approach

Keep playwright.config.ts as is

A mix of both

Create dedicated build, preview, and test targets

node_modules entry with playwright dependency

node_modules entrypoint without dependency

Use a dedicated script to start the playwright process with playwright dependencies in data

sourcegraph / sourcegraph-public-snapshot

Run playwright in CI #60881

With hermetic env var

Without --with-deps:

With alternative but unrecommended approach

Keep playwright.config.ts as is

A mix of both

Create dedicated build, preview, and test targets

node_modules entry with playwright dependency

node_modules entrypoint without dependency

Use a dedicated script to start the playwright process with playwright dependencies in data

Without `--with-deps`: