From a3a48b7605b13fa2c55e0b71b6ae916e4485cd45 Mon Sep 17 00:00:00 2001 From: Daniel Muller Date: Tue, 30 Apr 2024 10:01:23 -0600 Subject: [PATCH] Review comments addressed - Switch to `disable_sandbox` defaulting to true - Removed unused attribute from rule impl --- WORKSPACE.bazel | 7 ------- cypress/defs.bzl | 16 ++++++++-------- cypress/private/cypress_test.bzl | 3 --- docs/defs.md | 8 ++++---- e2e/workspace/MODULE.bazel | 2 +- 5 files changed, 13 insertions(+), 23 deletions(-) diff --git a/WORKSPACE.bazel b/WORKSPACE.bazel index 20b7e1a..9c1a10d 100644 --- a/WORKSPACE.bazel +++ b/WORKSPACE.bazel @@ -30,13 +30,6 @@ nodejs_register_toolchains( node_version = DEFAULT_NODE_VERSION, ) -load("@aspect_rules_cypress//cypress:repositories.bzl", "cypress_register_toolchains") - -cypress_register_toolchains( - name = "cypress", - cypress_version = "12.3.0", -) - # For running our own unit tests load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace") diff --git a/cypress/defs.bzl b/cypress/defs.bzl index 862913f..6a09a18 100644 --- a/cypress/defs.bzl +++ b/cypress/defs.bzl @@ -12,9 +12,9 @@ _cypress_test = rule( toolchains = js_binary_lib.toolchains + ["@aspect_rules_cypress//cypress:toolchain_type"], ) -def _cypress_test_macro(name, entry_point, cypress, **kwargs): +def _cypress_test_macro(name, entry_point, cypress, disable_sandbox, **kwargs): tags = kwargs.pop("tags", []) - if not kwargs.pop("allow_sandbox", False): + if disable_sandbox: tags.append("no-sandbox") _cypress_test( name = name, @@ -38,7 +38,7 @@ def _cypress_test_macro(name, entry_point, cypress, **kwargs): def cypress_test( name, cypress = "//:node_modules/cypress", - allow_sandbox = False, + disable_sandbox = True, browsers = [], **kwargs): """cypress_test runs the cypress CLI with the cypress toolchain. @@ -52,7 +52,7 @@ def cypress_test( Args: name: The name used for this rule and output files cypress: The cypress npm package which was already linked using an API like npm_link_all_packages. - allow_sandbox: Turn off sandboxing by default to allow electron to perform write operations. + disable_sandbox: Turn off sandboxing by default to allow electron to perform write operations. Cypress does not expose the underlying electron apis so we cannot alter the user app data directory to be within the bazel sandbox. @@ -92,7 +92,7 @@ def cypress_test( name = name, entry_point = entry_point, cypress = cypress, - allow_sandbox = allow_sandbox, + disable_sandbox = disable_sandbox, browsers = browsers, **kwargs ) @@ -101,7 +101,7 @@ def cypress_module_test( name, runner, cypress = "//:node_modules/cypress", - allow_sandbox = False, + disable_sandbox = True, browsers = [], **kwargs): """cypress_module_test creates a node environment which is hooked up to the cypress toolchain. @@ -133,7 +133,7 @@ def cypress_module_test( runner: JS file to call into the cypress module api See https://docs.cypress.io/guides/guides/module-api cypress: The cypress npm package which was already linked using an API like npm_link_all_packages. - allow_sandbox: Turn off sandboxing by default to allow electron to perform write operations. + disable_sandbox: Turn off sandboxing by default to allow electron to perform write operations. Cypress does not expose the underlying electron apis so we cannot alter the user app data directory to be within the bazel sandbox. @@ -165,7 +165,7 @@ def cypress_module_test( name = name, entry_point = runner, cypress = cypress, - allow_sandbox = allow_sandbox, + disable_sandbox = disable_sandbox, browsers = browsers, **kwargs ) diff --git a/cypress/private/cypress_test.bzl b/cypress/private/cypress_test.bzl index 6189111..1ad972c 100644 --- a/cypress/private/cypress_test.bzl +++ b/cypress/private/cypress_test.bzl @@ -4,9 +4,6 @@ load("@aspect_rules_js//js:libs.bzl", "js_binary_lib", "js_lib_helpers") load("@bazel_skylib//lib:dicts.bzl", "dicts") _attrs = dicts.add(js_binary_lib.attrs, { - "allow_sandbox": attr.bool( - default = False, - ), "browsers": attr.label_list( allow_files = True, ), diff --git a/docs/defs.md b/docs/defs.md index bd6c26f..26654e3 100644 --- a/docs/defs.md +++ b/docs/defs.md @@ -7,7 +7,7 @@ Public API re-exports ## cypress_module_test 
-cypress_module_test(name, runner, cypress, allow_sandbox, browsers, kwargs)
+cypress_module_test(name, runner, cypress, disable_sandbox, browsers, kwargs)
cypress_module_test creates a node environment which is hooked up to the cypress toolchain. @@ -43,7 +43,7 @@ In most scenarios, it is easier to use cypress_test. But in some scenarios, you | name | The name used for this rule and output files | none | | runner | JS file to call into the cypress module api See https://docs.cypress.io/guides/guides/module-api | none | | cypress | The cypress npm package which was already linked using an API like npm_link_all_packages. | `"//:node_modules/cypress"` | -| allow_sandbox | Turn off sandboxing by default to allow electron to perform write operations. Cypress does not expose the underlying electron apis so we cannot alter the user app data directory to be within the bazel sandbox.

From https://www.electronjs.org/docs/latest/api/app appData Per-user application data directory, which by default points to: %APPDATA% on Windows $XDG_CONFIG_HOME or ~/.config on Linux ~/Library/Application Support on macOS

Cypress may fail to connect on macos with errors like: Timed out waiting for the browser to connect. Retrying... Timed out waiting for the browser to connect. Retrying again... The browser never connected. Something is wrong. The tests cannot run. Aborting... | `False` | +| disable_sandbox | Turn off sandboxing by default to allow electron to perform write operations. Cypress does not expose the underlying electron apis so we cannot alter the user app data directory to be within the bazel sandbox.

From https://www.electronjs.org/docs/latest/api/app appData Per-user application data directory, which by default points to: %APPDATA% on Windows $XDG_CONFIG_HOME or ~/.config on Linux ~/Library/Application Support on macOS

Cypress may fail to connect on macos with errors like: Timed out waiting for the browser to connect. Retrying... Timed out waiting for the browser to connect. Retrying again... The browser never connected. Something is wrong. The tests cannot run. Aborting... | `True` | | browsers | A sequence of labels specifying the browsers to include. Usually, any dependency that you wish to be included in the runfiles tree should be included within the data attribute. However, data dependencies, by default, are copied to the Bazel output tree before being passed as inputs to runfiles.

This is not a good default behavior for browser since these typically come from external workspaces which cannot be symlinked into bazel-bin. Instead, we place them at the root of the runfiles tree. Use relative paths to construct account for this placement

e.g. ../../../BROWSER_WORKSPACE_NAME | `[]` | | kwargs | All other args from `js_test`. See https://github.com/aspect-build/rules_js/blob/main/docs/js_binary.md#js_test | none | @@ -53,7 +53,7 @@ In most scenarios, it is easier to use cypress_test. But in some scenarios, you ## cypress_test 
-cypress_test(name, cypress, allow_sandbox, browsers, kwargs)
+cypress_test(name, cypress, disable_sandbox, browsers, kwargs)
cypress_test runs the cypress CLI with the cypress toolchain. @@ -72,7 +72,7 @@ https://docs.cypress.io/guides/guides/command-line#What-you-ll-learn | :------------- | :------------- | :------------- | | name | The name used for this rule and output files | none | | cypress | The cypress npm package which was already linked using an API like npm_link_all_packages. | `"//:node_modules/cypress"` | -| allow_sandbox | Turn off sandboxing by default to allow electron to perform write operations. Cypress does not expose the underlying electron apis so we cannot alter the user app data directory to be within the bazel sandbox.

From https://www.electronjs.org/docs/latest/api/app appData Per-user application data directory, which by default points to: %APPDATA% on Windows $XDG_CONFIG_HOME or ~/.config on Linux ~/Library/Application Support on macOS

Cypress may fail to connect on macos with errors like: Timed out waiting for the browser to connect. Retrying... Timed out waiting for the browser to connect. Retrying again... The browser never connected. Something is wrong. The tests cannot run. Aborting... | `False` | +| disable_sandbox | Turn off sandboxing by default to allow electron to perform write operations. Cypress does not expose the underlying electron apis so we cannot alter the user app data directory to be within the bazel sandbox.

From https://www.electronjs.org/docs/latest/api/app appData Per-user application data directory, which by default points to: %APPDATA% on Windows $XDG_CONFIG_HOME or ~/.config on Linux ~/Library/Application Support on macOS

Cypress may fail to connect on macos with errors like: Timed out waiting for the browser to connect. Retrying... Timed out waiting for the browser to connect. Retrying again... The browser never connected. Something is wrong. The tests cannot run. Aborting... | `True` | | browsers | A sequence of labels specifying the browsers to include. Usually, any dependency that you wish to be included in the runfiles tree should be included within the data attribute. However, data dependencies, by default, are copied to the Bazel output tree before being passed as inputs to runfiles.

This is not a good default behavior for browser since these typically come from external workspaces which cannot be symlinked into bazel-bin. Instead, we place them at the root of the runfiles tree. Use relative paths to construct account for this placement

e.g. ../../../BROWSER_WORKSPACE_NAME | `[]` | | kwargs | All other args from `js_test`. See https://github.com/aspect-build/rules_js/blob/main/docs/js_binary.md#js_test | none | diff --git a/e2e/workspace/MODULE.bazel b/e2e/workspace/MODULE.bazel index eb9c815..8e970ef 100644 --- a/e2e/workspace/MODULE.bazel +++ b/e2e/workspace/MODULE.bazel @@ -5,7 +5,7 @@ local_path_override( ) cypress = use_extension("@aspect_rules_cypress//cypress:extensions.bzl", "cypress") -cypress.toolchain(cypress_version = "12.3.0") +cypress.toolchain(cypress_version = "12.12.0") use_repo(cypress, "cypress_toolchains") register_toolchains("@cypress_toolchains//:all")