bazelbuild / rules_webtesting

Bazel rules to allow testing against a browser with WebDriver.
Apache License 2.0
96 stars 56 forks source link

feat: add support for darwin m1 browsers #427

Closed devversion closed 3 years ago

devversion commented 3 years ago

rules_webtesting currently extracts browser archives as part of an build action. Additionally, the platform archive is determined at repository creation using the repository_ctx.os property. This breaks remote build execution, and also makes it impossible to support darwin ARM64 because the os name is equal regardless of CPU.

This is a good opportunity (and a necessary one) to move the extraction from a build action to the repository fetching (improving caching as build actions are prone to being invalidated more often). Additionally, with this approach we will be able to select the browser binaries at configuration time using select, allowing for darwin arm64, and proper remote build execution.

More details can be found in the description of the platform_archive rule.

devversion commented 3 years ago

cc. @josephperrott

mtrea commented 3 years ago

I can provide a review from the project admin side once this PR is ready (wasn't sure if Joey was still working on this)

devversion commented 3 years ago

@mtrea This PR would be ready for review. I've rebased it.

mtrea commented 3 years ago

I tested this locally and it looks like this breaks two of our tests compared to master:

You can confirm e.g. by running bazel test --runs_per_test=10 go/metadata/main:merger_test. Looking at the test log, it has:

Error merging file "/usr/[omitted]/merger_test.runfiles/io_bazel_rules_webtesting/testdata/named-files2.json": key "A" exists in both NamedFiles with different values

Can you PTAL to see what might be causing this? I wasn't able to find a culprit on my first pass.

devversion commented 3 years ago

@mtrea Good catch. thanks for trying it out. I have fixed both issues:

  1. The name of the Go binaries have changed, so the merger_test failed. I have reworked the test (there was a TODO for it as well) to no longer hard-code the binary name. The test will now also work on Windows and macOS!
  2. The Sauce test failed because I missed the prefix in the Sauce-Connect archives. I have updated the named_files definitions to include the prefix. Also I fixed the test so that it can work on Windows

    I wasn't able to fully get to run the Sauce test because there was an issue with the tunnel being established, but this seems totally unrelated.

Please have another look. thanks!

mtrea commented 3 years ago

Thanks for the quick fix; it's much appreciated. There's a known issue with the Sauce config, so I agree that AFAICT your fix is successful. Still need to follow up on my own to get the CI into a better state.

devversion commented 3 years ago

@mtrea Thanks for reviewing. Would it be possible to get a new release for this, so that the NodeJS bazel rules can support M1 browsers? Also note that the current latest release does not contain the actual release package built with the Copybara script.

mtrea commented 2 years ago

Yes, I have a separate email thread with Joey. We're checking to see if there are any other changes needed before I create another release.

Can you clarify what you mean by that last sentence? I know Copybara...what do you mean by the "actual release package"? I downloaded 0.3.4 and spot checked it and it didn't look obviously bad.

devversion commented 2 years ago

@mtrea If you look at https://github.com/bazelbuild/rules_webtesting/releases/tag/0.3.4 (see the assets below), then you can see that there is no rules_webtesting.tar.gz file attached like with previous other releases. e.g. https://github.com/bazelbuild/rules_webtesting/releases/tag/0.3.3 (see the assets)

Since there is no archive for 0.3.4, consumers will need to bring in the Go rules and build the binaries (like wsl) on their own. With the archive like in 0.3.3, binaries are prebuilt and consumers do not need any Go-related tooling..

mtrea commented 2 years ago

Thanks for spelling it out. The release process wasn't documented by the previous owners so there's a bit of trial and error. I released https://github.com/bazelbuild/rules_webtesting/releases/tag/0.3.5 but had to do the compilation steps semi-manually due to an issue with the releaser script. Please let me know if anything is amiss.

devversion commented 2 years ago

@mtrea Thanks for the new release. I have just tried the release package. There is an issue with it because the contents are wrapped in a folder called tmp/. There are workarounds for this but the release tarball should not need this.

$ tar -xvzf rules_webtesting.tar.gz
tmp/testdata/all-fields.json
tmp/web/internal/web_test_named_executable.bzl
tmp/go/webtest/webtest_test.go
tmp/third_party/chromedriver/LICENSE
tmp/go/webtest/
tmp/go/portpicker/port_picker.go
tmp/browsers/firefox-local.json
devversion commented 2 years ago

@mtrea I have created a pull request for improving the release script. That should ease the process in the future: #429