fix(karma): allow custom browsers to specify args (fixes #595) #2132

applmak · 2020-08-21T14:54:09Z

Today, the args as specified in the various manifests of the rules_webtesting
browsers never make it into the generated karma.conf.js. This results in these
arguments never being used when launching Chrome, preventing customization of
browsers such as window size, enabling remote debugging, or other flag-based
options. This PR fixes this by reading those arguments from the manifest, and
adding them to the browsers list in the generated karma.conf.js.

This PR also includes a small (manual) test that specifies an additional
argument in a custom browser, which then can be seen to be used via:

VERBOSE_LOGS=1 bazel run packages/karma/test/karma:testing_custom_chrome

Note the appearance of the additional flags in the new debug output.

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

See description above.

Issue Number: #595

What is the new behavior?

See description above.

Does this PR introduce a breaking change?

Perhaps! This might surprise people. If anyone has been using custom browsers up to now, they will suddenly see new arguments on their command-lines. However, given that the feature didn't really work before, I don't think that this is a big deal.

Yes
No

Other information

alexeagle

Great! thank you for contributing!

@gregmagolan was the author of this so I'd like his review too

alexeagle · 2020-08-21T14:58:17Z

packages/karma/test/karma/BUILD.bazel

@@ -41,3 +43,9 @@ karma_web_test_suite(
        "requirejs-config.js",
    ],
 )
+
+custom_browser(


how is this tested? to put it another way, if I take these changes to packages/karma/test but throw away the change to packages/karma/karma.conf.js will the CI be red?
Feels like maybe we're missing one more target here, like maybe a generated_file_test that asserts on what the generated karma conf will look like for the :testing karma_web_test_suite above

That's right. I was a bit concerned about the churn associated with a golden file test, but if that is the standard, I will create one.

Would you happen to know how to get a label to the generated .conf.js file?

it would be nice to test it in a better way but probably hard to sense whether the browser got a right initial dimensions or something.
I don't know the label offhand, if you use bazel query //the/package:all it will probably show up

I had to make the generated .conf.js a 'predeclared' output so as to write this test. I hope that's okay.

yeah that looks fine. Note you could also add the file to an Output Group to give a way to select the dynamic (non-predeclared) output

packages/karma/karma.conf.js

applmak · 2020-08-25T00:18:40Z

Ping!

alexeagle · 2020-08-25T18:23:13Z

packages/karma/test/karma/karma.conf.js.golden

@@ -0,0 +1,454 @@
+// Karma configuration


looks brittle that this golden is so long, probably more than we meant to assert on in this case.

I'd like to make a convenience rule for golden_content_test or sth so we can just say "make sure this line (± whitespace) appears in the file"

this is okay for now if the content is stable

It is stable. Should I file a bug about (perhaps) amending generated_file_test to include substring match capability?

I have added that capability. PTAL.

packages/karma/test/karma/BUILD.bazel

alexeagle

do you need help to get the tests green?

applmak · 2020-08-29T01:47:31Z

Ah, looks like it's not as stable as I would have wished. I'll make that improvement to generated_file_test first.

applmak · 2020-08-29T03:55:14Z

Yep! It looks like I will need help. I'm not sure of the mechanism that seems to be excluding the :testing_chromium-local from buildkite/rules-nodejs-nodejs, but when it tried to run :testing_custom_chrome, it errors with a missing .so.

applmak · 2020-09-02T22:23:41Z

Ping!

alexeagle · 2020-09-03T14:58:44Z

Hmm, I don't have time to dig in right now. The error for anyone following along:

ERROR [launcher]: Cannot start ChromeHeadless
  | /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1531/execroot/build_bazel_rules_nodejs/bazel-out/k8-fastbuild/bin/packages/karma/test/karma/testing_custom_chrome.sh.runfiles/io_bazel_rules_webtesting/third_party/chromium/chromium.out/chrome-linux/chrome: error while loading shared libraries: libatk-bridge-2.0.so.0: cannot open shared object file: No such file or directory

Could this be due to the custom_chrome.json enabling some options that cause Chromium to dynamic-link more libraries than before? We only installed a minimal set on our CI workers - ideally you'd find a way to assert that the custom browser args are being honored, but without introducing new dependencies on the underlying machine.

(Also WDYT about the substring mode accepting a string attribute with the content to match, rather than (or in addition to) accepting a golden file input? In a lot of cases you just want the equivalent of assert_contains which would mean a lot of tiny one-line files)

applmak · 2020-09-07T19:27:34Z

So, these tests have never worked on CircleCI. The CircleCL script executes:

bazel test --flaky_test_attempts=3 --build_tests_only --local_test_jobs=12 --show_progress_rate_limit=5 --curses=yes --color=yes --terminal_columns=143 --show_timestamps --verbose_failures --jobs=30 --announce_rc --experimental_repository_cache_hardlinks --disk_cache= --sandbox_tmpfs_path=/tmp --experimental_build_event_json_file_path_conversion=false --build_event_json_file=/tmp/tmperwd7qg1/test_bep.json --test_tag_filters=-e2e,-examples,-manual,-browser:chromium-local,-browser:firefox-local,-cypress --test_env=HOME --test_env=USE_BAZEL_VERSION --sandbox_writable_path=/var/lib/buildkite-agent/.cache/bazelisk -- //... /

which removes tests that are tagged browser:chromium-local.

Running

bazel query --output=build packages/karma/test/karma:all

shows the generated test rule as:

web_test(
  name = "testing_custom_chrome",
  tags = ["browser:custom_chrome", "custom_chrome", "ibazel_notify_changes", "native"],
  generator_name = "testing",
  generator_function = "karma_web_test_suite",
  generator_location = "packages/karma/test/karma/BUILD.bazel:19:1",
  size = "large",
  browser = "//packages/karma/test/karma:custom_chrome",
  config = "@io_bazel_rules_webtesting//web:default_config",
  data = [],
  launcher = "//packages/karma/test/karma:testing_wrapped_test",
  test = "//packages/karma/test/karma:testing_wrapped_test",
)

which means it does not get excluded from the circlecl integration test.

So: either, I can add a misleading tag to the test suite (something like browser:chromium-local), or I can extend the circlecl tag exclusion to include browser:custom_chrome. LMK what you want.

applmak · 2020-09-07T19:32:09Z

I found the configuration at .buildci/presubmit.yml and updated it to include an exclusion for tests with the tag 'browser:custom_chrome'.

PTAL.

applmak · 2020-09-11T01:17:56Z

Ping!

applmak · 2020-09-15T01:36:09Z

Pong!

applmak · 2020-09-17T01:13:19Z

Pingigidipong!

Today, the args as specified in the various manifests of the rules_webtesting browsers never make it into the generated karma.conf.js. This results in these arguments never being used when launching Chrome, preventing customization of browsers such as window size, enabling remote debugging, or other flag-based options. This PR fixes this by reading those arguments from the manifest, and adding them to the browsers list in the generated karma.conf.js. Also, this PR includes a change to generated_file_test to allow a golden file to represent a substring of the generated output. Also Also: This PR includes a golden file test that verified that the generated karma.conf.js does read in the specified value. Furthermore, the effect of this can be verified manually via: ``` VERBOSE_LOGS=1 bazel run packages/karma/test/karma:testing_custom_chrome ``` Note the appearance of the additional flags in the new debug output.

alexeagle · 2020-09-23T18:31:25Z

CI rerun still failed, trying a rebase on stable/HEAD to see if it's green

alexeagle · 2020-09-23T20:28:32Z

Thanks for pinging :)

applmak requested review from alexeagle, gregmagolan, mattem and soldair as code owners August 21, 2020 14:54

googlebot added the cla: yes label Aug 21, 2020

alexeagle reviewed Aug 21, 2020

View reviewed changes

alexeagle reviewed Aug 25, 2020

View reviewed changes

packages/karma/test/karma/BUILD.bazel Outdated Show resolved Hide resolved

alexeagle approved these changes Aug 25, 2020

View reviewed changes

alexeagle merged commit 5a58030 into bazel-contrib:stable Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(karma): allow custom browsers to specify args (fixes #595) #2132

fix(karma): allow custom browsers to specify args (fixes #595) #2132

applmak commented Aug 21, 2020

alexeagle left a comment

alexeagle Aug 21, 2020

applmak Aug 21, 2020

alexeagle Aug 21, 2020

applmak Aug 23, 2020

alexeagle Aug 25, 2020

applmak commented Aug 25, 2020

alexeagle Aug 25, 2020

applmak Aug 29, 2020

applmak Aug 29, 2020

alexeagle left a comment

applmak commented Aug 29, 2020

applmak commented Aug 29, 2020

applmak commented Sep 2, 2020

alexeagle commented Sep 3, 2020

applmak commented Sep 7, 2020

applmak commented Sep 7, 2020

applmak commented Sep 11, 2020

applmak commented Sep 15, 2020

applmak commented Sep 17, 2020

alexeagle commented Sep 23, 2020

alexeagle commented Sep 23, 2020

fix(karma): allow custom browsers to specify args (fixes #595) #2132

fix(karma): allow custom browsers to specify args (fixes #595) #2132

Conversation

applmak commented Aug 21, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alexeagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

applmak commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle left a comment

Choose a reason for hiding this comment

applmak commented Aug 29, 2020

applmak commented Aug 29, 2020

applmak commented Sep 2, 2020

alexeagle commented Sep 3, 2020

applmak commented Sep 7, 2020

applmak commented Sep 7, 2020

applmak commented Sep 11, 2020

applmak commented Sep 15, 2020

applmak commented Sep 17, 2020

alexeagle commented Sep 23, 2020

alexeagle commented Sep 23, 2020