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

feat: Create js_run_devserver target in macro #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrmeku
Copy link
Contributor

@mrmeku mrmeku commented Aug 17, 2024

Every cypress_test the macro now generates
a supplementary target [name].open which runs
cypress using js_run_devserver. This allows users
to use ibazel to interactively update their tests.


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

Test plan

  • Covered by existing test cases

cypress_test macro now creates another target with name [name].open. This target runs cypress open using js_run_devserver. It is meant to be run with ibazel so that your tests can be updated interactively.

Copy link
Contributor Author

mrmeku commented Aug 17, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @mrmeku and the rest of your teammates on Graphite Graphite

cypress_files = ctx.toolchains["@aspect_rules_cypress//cypress:toolchain_type"].cypressinfo.tool_files

files = ctx.files.data[:] + cypress_files + ctx.files.browsers

if ctx.attr.chdir:
cypress_bin = "/".join([".." for _ in ctx.attr.chdir.split("/")] + [cypress_bin])
# TODO: Augment rules_js to expose EXECROOT before setting fixed_env. Then use that variable.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregmagolan What do you think about this TODO? Is that a sane change to rules_js? Is this a sane way to lookup the execroot in the first place?


launcher = js_binary_lib.create_launcher(
ctx,
log_prefix_rule_set = "aspect_rules_cypess",
log_prefix_rule = "cypress_node_test",
fixed_args = ctx.attr.fixed_args,
fixed_env = {
"CYPRESS_RUN_BINARY": cypress_bin,
"HOME": "$$TEST_TMPDIR",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TEST_TMPDIR isn't defined in bazel run so I removed these. I'd like to keep them and use something like or if they aren't defined. Any suggestions @gregmagolan ?

Copy link
Member

Choose a reason for hiding this comment

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

Why did we set HOME in the first place? Guessing cypress was trying to write to the home directory when it the sandbox and failing?

@mrmeku mrmeku force-pushed the mrmeku-cypress.open branch 2 times, most recently from 973b560 to 98c52d1 Compare August 17, 2024 18:03
For every `cypress_test` the macro now generates
a supplementary target `[name].open` which runs
cypress using js_run_devserver. This allows users
to use ibazel to interactively update their tests.
if ctx.attr.chdir:
cypress_bin = "/".join([".." for _ in ctx.attr.chdir.split("/")] + [cypress_bin])
# TODO: Augment rules_js to expose JS__EXECROOT before setting fixed_env. Then use that variable.
execroot = "$$( realpath \"$${BASH_SOURCE[0]}\" | sed -E 's/^(.*\\/execroot\\/[^/]+).*/\\1/')"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Dep on realpath not great and it will also escape the sandbox this way.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably augment rules_js to support what you want here instead of this line

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