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

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

Merged
merged 1 commit into from
Sep 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ tasks:
# on bazelci apt-get fails with permission denied and there is no sudo
# command to switch to root.
# TODO(gregmagolan): figure out how to install missing shared libs
- "--test_tag_filters=-e2e,-examples,-manual,-browser:chromium-local,-browser:firefox-local,-cypress"
- "--test_tag_filters=-e2e,-examples,-manual,-browser:chromium-local,-browser:firefox-local,-browser:custom_chrome,-cypress"
test_targets:
- "//..."
# //internal/node/test:nodejs_toolchain_linux_amd64_test is a "manual" test that must be run
Expand Down Expand Up @@ -137,7 +137,7 @@ tasks:
# on bazelci apt-get fails with permission denied and there is no sudo
# command to switch to root.
# TODO(gregmagolan): figure out how to install missing shared libs
- "--test_tag_filters=-e2e,-examples,-manual,-browser:chromium-local,-browser:firefox-local,-cypress"
- "--test_tag_filters=-e2e,-examples,-manual,-browser:chromium-local,-browser:firefox-local,-browser:custom_chrome,-cypress"
test_targets:
- "//..."
# //internal/node/test:nodejs_toolchain_linux_amd64_test is a "manual" test that must be run
Expand All @@ -158,7 +158,7 @@ tasks:
# on bazelci apt-get fails with permission denied and there is no sudo
# command to switch to root.
# TODO(gregmagolan): figure out how to install missing shared libs
- "--test_tag_filters=-e2e,-examples,-manual,-browser:chromium-local,-browser:firefox-local,-cypress"
- "--test_tag_filters=-e2e,-examples,-manual,-browser:chromium-local,-browser:firefox-local,-browser:custom_chrome,-cypress"
test_targets:
- "//..."
ubuntu1804_e2e:
Expand Down
32 changes: 31 additions & 1 deletion internal/generated_file_test/bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,23 @@ const path = require('path');
import * as unidiff from 'unidiff/unidiff';
const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);

function findGoldenInGenerated(golden, actual) {
const goldenLines = golden.split(/[\r\n]+/g).map(l => l.trim());
const actualLines = actual.split(/[\r\n]+/g).map(l => l.trim());
// Note: this is not the fastest subsequence algorithm.
nextActualLine: for (let i = 0; i < actualLines.length; i++) {
for (let j = 0; j < goldenLines.length; j++) {
if (actualLines[i + j] !== goldenLines[j]) {
continue nextActualLine;
}
}
// A match!
return true;
}
// No match.
return false;
}

function main(args) {
const [mode, golden_no_debug, golden_debug, actual] = args;
const actualPath = runfiles.resolveWorkspaceRelative(actual);
Expand All @@ -23,7 +40,7 @@ function main(args) {
return 0;
}
if (mode === '--verify') {
// Generated does not match golden
// Compare the generated file to the golden file.
const diff = unidiff.diffLines(goldenContents, actualContents);
let prettyDiff =
unidiff.formatLines(diff, {aname: `[workspace]/${golden}`, bname: `[bazel-out]/${actual}`});
Expand All @@ -41,6 +58,19 @@ If the bazel-out content is correct, you can update the workspace file by runnin
`);
return 1;
}
if (mode === '--substring') {
// Verify that the golden file is contained _somewhere_ in the generated
// file.
const diff = findGoldenInGenerated(goldenContents, actualContents);
if (diff) {
console.error(`Unable to find golden contents inside of the the generated file:

${goldenContents}
`)
return 1;
}
return 0;
}
throw new Error('unknown mode', mode);
}

Expand Down
28 changes: 18 additions & 10 deletions internal/generated_file_test/generated_file_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

load("@build_bazel_rules_nodejs//internal/node:node.bzl", "nodejs_binary", "nodejs_test")

def generated_file_test(name, generated, src, src_dbg = None, **kwargs):
def generated_file_test(name, generated, src, substring_search = False, src_dbg = None, **kwargs):
"""Tests that a file generated by Bazel has identical content to a file in the workspace.

This is useful for testing, where a "snapshot" or "golden" file is checked in,
Expand All @@ -12,6 +12,8 @@ def generated_file_test(name, generated, src, src_dbg = None, **kwargs):
name: Name of the rule.
generated: a Label of the output file generated by another rule
src: Label of the source file in the workspace
substring_search: When true, creates a test that will fail only if the golden file is not found
anywhere within the generated file. Note that the .update rule is not generated in substring mode.
src_dbg: if the build uses `--compilation_mode dbg` then some rules will produce different output.
In this case you can specify what the dbg version of the output should look like
**kwargs: extra arguments passed to the underlying nodejs_test or nodejs_binary
Expand All @@ -27,16 +29,22 @@ def generated_file_test(name, generated, src, src_dbg = None, **kwargs):
nodejs_test(
name = name,
entry_point = "@build_bazel_rules_nodejs//internal/generated_file_test:bundle.js",
templated_args = ["--verify", loc % src, loc % src_dbg, loc % generated],
templated_args = [
"--substring" if substring_search else "--verify",
loc % src,
loc % src_dbg,
loc % generated,
],
data = data,
**kwargs
)

nodejs_binary(
name = name + ".update",
testonly = True,
entry_point = "@build_bazel_rules_nodejs//internal/generated_file_test:bundle.js",
templated_args = ["--out", loc % src, loc % src_dbg, loc % generated],
data = data,
**kwargs
)
if not substring_search:
nodejs_binary(
name = name + ".update",
testonly = True,
entry_point = "@build_bazel_rules_nodejs//internal/generated_file_test:bundle.js",
templated_args = ["--out", loc % src, loc % src_dbg, loc % generated],
data = data,
**kwargs
)
14 changes: 12 additions & 2 deletions packages/karma/karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,13 +321,23 @@ try {
webTestNamedFiles['CHROMIUM']}' in runfiles`);
}
}
// Read any additional chrome options (as specified by the
// rules_webtesting manifest).
const chromeOptions = (webTestMetadata['capabilities'] || {})['goog:chromeOptions'];
const additionalArgs = (chromeOptions ? chromeOptions['args'] : []).filter(arg => {
applmak marked this conversation as resolved.
Show resolved Hide resolved
// We never want to 'run' Chrome in headless mode.
return arg != '--headless';
});
const browser = process.env['DISPLAY'] ? 'Chrome' : 'ChromeHeadless';
if (!supportChromeSandboxing()) {
const launcher = 'CustomChromeWithoutSandbox';
conf.customLaunchers = {[launcher]: {base: browser, flags: ['--no-sandbox']}};
conf.customLaunchers =
{[launcher]: {base: browser, flags: ['--no-sandbox', ...additionalArgs]}};
conf.browsers.push(launcher);
} else {
conf.browsers.push(browser);
const launcher = 'CustomChrome';
conf.customLaunchers = {[launcher]: {base: browser, flags: additionalArgs}};
conf.browsers.push(launcher);
}
}
if (webTestNamedFiles['FIREFOX']) {
Expand Down
8 changes: 4 additions & 4 deletions packages/karma/karma_web_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,7 @@ def _find_dep(ctx, suffix):

# Generates the karma configuration file for the rule
def _write_karma_config(ctx, files, amd_names_shim):
configuration = ctx.actions.declare_file(
"%s.conf.js" % ctx.label.name,
sibling = ctx.outputs.executable,
)
configuration = ctx.outputs.configuration

config_file = None

Expand Down Expand Up @@ -325,6 +322,9 @@ _karma_web_test = rule(
test = True,
executable = True,
attrs = KARMA_WEB_TEST_ATTRS,
outputs = {
"configuration": "%{name}.conf.js",
},
)

def karma_web_test(
Expand Down
16 changes: 16 additions & 0 deletions packages/karma/test/karma/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load("@build_bazel_rules_nodejs//:index.bzl", "generated_file_test")
load("@io_bazel_rules_webtesting//web:web.bzl", "custom_browser")
load("//packages/karma:index.bzl", "karma_web_test_suite")

karma_web_test_suite(
Expand All @@ -26,6 +28,7 @@ karma_web_test_suite(
browsers = [
"@io_bazel_rules_webtesting//browsers:chromium-local",
"@io_bazel_rules_webtesting//browsers:firefox-local",
":custom_chrome",
],
static_files = [
"unnamed-amd-module.js",
Expand All @@ -41,3 +44,16 @@ karma_web_test_suite(
"requirejs-config.js",
],
)

custom_browser(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

name = "custom_chrome",
browser = "@io_bazel_rules_webtesting//browsers:chromium-local",
metadata = "custom_chrome.json",
)

generated_file_test(
name = "test_custom_chrome_karma_conf",
src = "karma.conf.js.golden",
generated = "testing_wrapped_test.conf.js",
substring_search = True,
)
11 changes: 11 additions & 0 deletions packages/karma/test/karma/custom_chrome.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"capabilities": {
"goog:chromeOptions": {
"args": [
"--remote-debugging-port=9222",
"--headless",
"--use-gl=swiftshader-webgl"
]
}
}
}
5 changes: 5 additions & 0 deletions packages/karma/test/karma/karma.conf.js.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const chromeOptions = (webTestMetadata['capabilities'] || {})['goog:chromeOptions'];
const additionalArgs = (chromeOptions ? chromeOptions['args'] : []).filter(arg => {
// We never want to 'run' Chrome in headless mode.
return arg != '--headless';
});