From bdd7e7682d01484bebd270c24cd24fc4312c3a96 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Tue, 24 Mar 2020 16:28:45 -0700 Subject: [PATCH] fixup! feat(builtin): enable coverage on nodejs_test --- common.bazelrc | 3 +- internal/coverage/BUILD.bazel | 4 + internal/coverage/lcov_merger.js | 29 +++-- internal/coverage/lcov_merger.ts | 29 +++-- internal/node/launcher.sh | 76 ++++++++---- internal/node/node.bzl | 15 +-- packages/jasmine/src/BUILD.bazel | 2 - packages/jasmine/src/coverage_runner.js | 114 ------------------ packages/jasmine/src/index.from_src.bzl | 1 - packages/jasmine/src/jasmine_node_test.bzl | 2 +- .../typescript/src/internal/build_defs.bzl | 4 - 11 files changed, 105 insertions(+), 174 deletions(-) delete mode 100644 packages/jasmine/src/coverage_runner.js diff --git a/common.bazelrc b/common.bazelrc index aba5e60d7a..0f94d61c26 100644 --- a/common.bazelrc +++ b/common.bazelrc @@ -76,7 +76,8 @@ common --experimental_allow_incremental_repository_updates build --incompatible_strict_action_env run --incompatible_strict_action_env -# when running `bazel coverage` ensure that the test targets are instrumented +# When running `bazel coverage` --instrument_test_targets needs to be set in order to +# collect coverage information from test targets coverage --instrument_test_targets # Load any settings specific to the current user. diff --git a/internal/coverage/BUILD.bazel b/internal/coverage/BUILD.bazel index 982989ade8..6e89663f6c 100644 --- a/internal/coverage/BUILD.bazel +++ b/internal/coverage/BUILD.bazel @@ -1,5 +1,9 @@ load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary") +exports_files([ + "lcov_merger.js", +]) + # BEGIN-INTERNAL load("@npm_bazel_typescript//:index.from_src.bzl", "checked_in_ts_library") diff --git a/internal/coverage/lcov_merger.js b/internal/coverage/lcov_merger.js index 40d5675e53..7dcc169be1 100644 --- a/internal/coverage/lcov_merger.js +++ b/internal/coverage/lcov_merger.js @@ -37,24 +37,23 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge const crypto = require("crypto"); const fs = require("fs"); const path = require("path"); - function getArg(argv, key) { - return argv.find(a => a === key).split('=')[1]; + function _getArg(argv, key) { + return argv.find(a => a.startsWith(key)).split('=')[1]; } /** * This is designed to collect the coverage of one target, since in nodejs * and using NODE_V8_COVERAGE it may produce more than one coverage file, however bazel expects * there to be only one lcov file. So this collects up the v8 coverage json's merges them and * converts them to lcov for bazel to pick up later. - * any tool reporting coverage not just jasmine */ function main() { return __awaiter(this, void 0, void 0, function* () { - // see here for what args are passed in + // Using the standard args for a bazel lcov merger binary: // https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh#L175-L181 const argv = process.argv; - const coverageDir = getArg(argv, 'coverage_dir'); - const outputFile = getArg(argv, 'output_file'); - const sourceFileManifest = getArg(argv, 'source_file_manifest'); + const coverageDir = _getArg(argv, '--coverage_dir='); + const outputFile = _getArg(argv, '--output_file='); + const sourceFileManifest = _getArg(argv, '--source_file_manifest='); const tmpdir = process.env.TEST_TMPDIR; if (!sourceFileManifest || !tmpdir || !outputFile) { throw new Error(); @@ -68,12 +67,12 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge const includes = instrumentedSourceFiles // the manifest may include files such as .bash so we want to reduce that down to the set // we can run coverage on in JS - .filter(f => ['.js', '.jsx', 'cjs', '.ts', '.tsx', '.mjs'].includes(path.extname(f))) + .filter(f => ['.js', '.jsx', '.cjs', '.ts', '.tsx', '.mjs'].includes(path.extname(f))) .map(f => { // at runtime we only run .js or .mjs // meaning that the coverage written by v8 will only include urls to .js or .mjs // so the source files need to be mapped from their input to output extensions - // TODO: how do we know what source files produce .mjs or cjs? + // TODO: how do we know what source files produce .mjs or .cjs? const p = path.parse(f); let targetExt; switch (p.ext) { @@ -85,7 +84,17 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge return path.format(Object.assign(Object.assign({}, p), { base: undefined, ext: targetExt })); }); // only require in c8 when we're actually going to do some coverage - const c8 = require('c8'); + let c8; + try { + c8 = require('c8'); + } + catch (e) { + if (e.code == 'MODULE_NOT_FOUND') { + console.error('ERROR: c8 npm package is required for bazel coverage'); + process.exit(1); + } + throw e; + } // see https://github.com/bcoe/c8/blob/master/lib/report.js // for more info on this function // TODO: enable the --all functionality diff --git a/internal/coverage/lcov_merger.ts b/internal/coverage/lcov_merger.ts index aa24772579..1185dd7c8c 100644 --- a/internal/coverage/lcov_merger.ts +++ b/internal/coverage/lcov_merger.ts @@ -19,8 +19,8 @@ import * as crypto from 'crypto'; import * as fs from 'fs'; import * as path from 'path'; -function getArg(argv: string[], key: string): string { - return argv.find(a => a === key)!.split('=')[1]; +function _getArg(argv: string[], key: string): string { + return argv.find(a => a.startsWith(key))!.split('=')[1]; } /** @@ -28,15 +28,14 @@ function getArg(argv: string[], key: string): string { * and using NODE_V8_COVERAGE it may produce more than one coverage file, however bazel expects * there to be only one lcov file. So this collects up the v8 coverage json's merges them and * converts them to lcov for bazel to pick up later. - * any tool reporting coverage not just jasmine */ async function main() { - // see here for what args are passed in + // Using the standard args for a bazel lcov merger binary: // https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh#L175-L181 const argv = process.argv; - const coverageDir = getArg(argv, 'coverage_dir'); - const outputFile = getArg(argv, 'output_file'); - const sourceFileManifest = getArg(argv, 'source_file_manifest'); + const coverageDir = _getArg(argv, '--coverage_dir='); + const outputFile = _getArg(argv, '--output_file='); + const sourceFileManifest = _getArg(argv, '--source_file_manifest='); const tmpdir = process.env.TEST_TMPDIR; if (!sourceFileManifest || !tmpdir || !outputFile) { @@ -44,7 +43,6 @@ async function main() { } const instrumentedSourceFiles = fs.readFileSync(sourceFileManifest).toString('utf8').split('\n'); - // c8 will name the output report file lcov.info // so we give it a dir that it can write to // later on we'll move and rename it into output_file as bazel expects @@ -55,12 +53,12 @@ async function main() { instrumentedSourceFiles // the manifest may include files such as .bash so we want to reduce that down to the set // we can run coverage on in JS - .filter(f => ['.js', '.jsx', 'cjs', '.ts', '.tsx', '.mjs'].includes(path.extname(f))) + .filter(f => ['.js', '.jsx', '.cjs', '.ts', '.tsx', '.mjs'].includes(path.extname(f))) .map(f => { // at runtime we only run .js or .mjs // meaning that the coverage written by v8 will only include urls to .js or .mjs // so the source files need to be mapped from their input to output extensions - // TODO: how do we know what source files produce .mjs or cjs? + // TODO: how do we know what source files produce .mjs or .cjs? const p = path.parse(f); let targetExt; switch (p.ext) { @@ -74,7 +72,16 @@ async function main() { }); // only require in c8 when we're actually going to do some coverage - const c8 = require('c8'); + let c8; + try { + c8 = require('c8'); + } catch (e) { + if (e.code == 'MODULE_NOT_FOUND') { + console.error('ERROR: c8 npm package is required for bazel coverage'); + process.exit(1); + } + throw e; + } // see https://github.com/bcoe/c8/blob/master/lib/report.js // for more info on this function // TODO: enable the --all functionality diff --git a/internal/node/launcher.sh b/internal/node/launcher.sh index eabcfecfc5..d91f57199f 100644 --- a/internal/node/launcher.sh +++ b/internal/node/launcher.sh @@ -90,9 +90,6 @@ fi export RUNFILES # --- end RUNFILES initialization --- -# TODO: debug - remove this -set -x - TEMPLATED_env_vars # Note: for debugging it is useful to see what files are actually present @@ -161,6 +158,7 @@ MAIN=$(rlocation "TEMPLATED_loader_script") readonly repository_args=$(rlocation "TEMPLATED_repository_args") readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script") +readonly lcov_merger_script=$(rlocation "TEMPLATED_lcov_merger_script") node_patches_script=$(rlocation "TEMPLATED_node_patches_script") require_patch_script=${BAZEL_NODE_PATCH_REQUIRE} @@ -224,31 +222,67 @@ export BAZEL_PATCH_ROOT=$(dirname $PWD) # a binary fails to run. Otherwise any failure would make such a test # fail before we could assert that we expected that failure. readonly EXPECTED_EXIT_CODE="TEMPLATED_expected_exit_code" -if [ "${EXPECTED_EXIT_CODE}" -eq "0" ]; then - # Replace the current process (bash) with a node process. - # This means that stdin, stdout, signals, etc will be transparently - # handled by the node process. - # If we had merely forked a child process here, we'd be responsible - # for forwarding those OS interactions. - exec "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} - # exec terminates execution of this shell script, nothing later will run. + +if [[ -n "${COVERAGE_DIR:-}" ]]; then + if [[ -n "${VERBOSE_LOGS:-}" ]]; then + echo "Turning on node coverage with NODE_V8_COVERAGE=${COVERAGE_DIR}" + fi + # Setting NODE_V8_COVERAGE=${COVERAGE_DIR} causes NodeJS to write coverage + # information our to the COVERAGE_DIR once the process exits + export NODE_V8_COVERAGE=${COVERAGE_DIR} fi +# Bash does not forward termination signals to any child process when +# running in docker so need to manually trap and forward the signals +_term() { + kill -TERM "$child" 2>/dev/null +} + +_int() { + kill -INT "$child" 2>/dev/null +} + +# Execute the main program set +e -"${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} +"${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} <&0 & +child=$! +trap _term SIGTERM +trap _int SIGINT +wait "$child" RESULT="$?" set -e -if [ ${RESULT} != ${EXPECTED_EXIT_CODE} ]; then - echo "Expected exit code to be ${EXPECTED_EXIT_CODE}, but got ${RESULT}" >&2 - if [ "${RESULT}" -eq "0" ]; then - # This exit code is handled specially by Bazel: - # https://github.com/bazelbuild/bazel/blob/486206012a664ecb20bdb196a681efc9a9825049/src/main/java/com/google/devtools/build/lib/util/ExitCode.java#L44 - readonly BAZEL_EXIT_TESTS_FAILED=3; - exit ${BAZEL_EXIT_TESTS_FAILED} +if [ "${EXPECTED_EXIT_CODE}" != "0" ]; then + if [ ${RESULT} != ${EXPECTED_EXIT_CODE} ]; then + echo "Expected exit code to be ${EXPECTED_EXIT_CODE}, but got ${RESULT}" >&2 + if [ "${RESULT}" -eq "0" ]; then + # This exit code is handled specially by Bazel: + # https://github.com/bazelbuild/bazel/blob/486206012a664ecb20bdb196a681efc9a9825049/src/main/java/com/google/devtools/build/lib/util/ExitCode.java#L44 + readonly BAZEL_EXIT_TESTS_FAILED=3; + exit ${BAZEL_EXIT_TESTS_FAILED} + fi + else + exit 0 + fi +fi + +# Post process the coverage information after the process has exited +if [[ -n "${COVERAGE_DIR:-}" ]]; then + if [[ -n "${VERBOSE_LOGS:-}" ]]; then + echo "Running coverage lcov merger script with arguments" + echo " --coverage_dir="${COVERAGE_DIR}"" + echo " --output_file="${COVERAGE_OUTPUT_FILE}"" + echo " --source_file_manifest="${COVERAGE_MANIFEST}"" + fi + + set +e + "${node}" "${lcov_merger_script}" --coverage_dir="${COVERAGE_DIR}" --output_file="${COVERAGE_OUTPUT_FILE}" --source_file_manifest="${COVERAGE_MANIFEST}" + RESULT="$?" + set -e + + if [ ${RESULT} -ne 0 ]; then + exit ${RESULT} fi -else - exit 0 fi exit ${RESULT} \ No newline at end of file diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 2b8c3613f6..53f906bcee 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -188,14 +188,6 @@ def _nodejs_binary_impl(ctx): elif k in ctx.configuration.default_shell_env.keys(): env_vars += "export %s=\"%s\"\n" % (k, ctx.configuration.default_shell_env[k]) - # indicates that this was run with `bazel coverage` - if ctx.configuration.coverage_enabled: - # indicates that we have files to instrumented somewhere in the deps - if (ctx.coverage_instrumented() or any([ctx.coverage_instrumented(dep) for dep in ctx.attr.data])): - # we export NODE_V8_COVERAGE here to tell V8 to collect coverage - # then when the nodejs process exists it'll write it to COVERAGE_DIR - env_vars += "export NODE_V8_COVERAGE=$COVERAGE_DIR\n" - expected_exit_code = 0 if hasattr(ctx.attr, "expected_exit_code"): expected_exit_code = ctx.attr.expected_exit_code @@ -215,6 +207,7 @@ def _nodejs_binary_impl(ctx): node_tool_files.append(ctx.file._link_modules_script) node_tool_files.append(ctx.file._runfiles_helper_script) node_tool_files.append(ctx.file._node_patches_script) + node_tool_files.append(ctx.file._lcov_merger_script) node_tool_files.append(node_modules_manifest) is_builtin = ctx.attr._node.label.workspace_name in ["nodejs_%s" % p for p in BUILT_IN_NODE_PLATFORMS] @@ -240,6 +233,7 @@ def _nodejs_binary_impl(ctx): "TEMPLATED_entry_point_manifest_path": _to_manifest_path(ctx, ctx.file.entry_point), "TEMPLATED_env_vars": env_vars, "TEMPLATED_expected_exit_code": str(expected_exit_code), + "TEMPLATED_lcov_merger_script": _to_manifest_path(ctx, ctx.file._lcov_merger_script), "TEMPLATED_link_modules_script": _to_manifest_path(ctx, ctx.file._link_modules_script), "TEMPLATED_loader_script": _to_manifest_path(ctx, ctx.outputs.loader_script), "TEMPLATED_modules_manifest": _to_manifest_path(ctx, node_modules_manifest), @@ -548,6 +542,10 @@ Predefined genrule variables are not supported in this context. default = Label("//internal/node:launcher.sh"), allow_single_file = True, ), + "_lcov_merger_script": attr.label( + default = Label("//internal/coverage:lcov_merger.js"), + allow_single_file = True, + ), "_link_modules_script": attr.label( default = Label("//internal/linker:index.js"), allow_single_file = True, @@ -619,7 +617,6 @@ nodejs_test = rule( "_lcov_merger": attr.label( executable = True, default = Label("@build_bazel_rules_nodejs//internal/coverage:lcov_merger_sh"), - # default = Label("@build_bazel_rules_nodejs//internal/coverage:lcov_merger_js"), cfg = "target", ), }), diff --git a/packages/jasmine/src/BUILD.bazel b/packages/jasmine/src/BUILD.bazel index 20c6cd9fda..620e07bc82 100644 --- a/packages/jasmine/src/BUILD.bazel +++ b/packages/jasmine/src/BUILD.bazel @@ -42,7 +42,6 @@ stardoc( filegroup( name = "package_contents", srcs = [ - "coverage_runner.js", "index.bzl", "index.js", "jasmine_node_test.bzl", @@ -55,7 +54,6 @@ js_library( name = "jasmine__pkg", package_name = "@bazel/jasmine", srcs = [ - "coverage_runner.js", "index.js", "jasmine_runner.js", ], diff --git a/packages/jasmine/src/coverage_runner.js b/packages/jasmine/src/coverage_runner.js deleted file mode 100644 index 25fe7bffd7..0000000000 --- a/packages/jasmine/src/coverage_runner.js +++ /dev/null @@ -1,114 +0,0 @@ -/** - * @license - * Copyright 2017 The Bazel Authors. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -const fs = require('fs'); -const path = require('path'); -const crypto = require('crypto'); -const childProcess = require('child_process'); - -/** - * This is designed to collect the coverage of one target, since in nodejs - * and using NODE_V8_COVERAGE it may produce more than one coverage file, however bazel expects - * there to be only one lcov file. So this collects up the v8 coverage json's merges them and - * converts them to lcov for bazel to pick up later. - * TODO: The functionality in this script could be put into _lcov_merger then it would apply to - * any tool reporting coverage not just jasmine - */ -async function main() { - const result = childProcess.spawnSync( - process.execPath, - [...process.execArgv, __dirname + '/jasmine_runner.js', ...process.argv.slice(2)], - {stdio: 'inherit'}); - - const exitCode = result.status; - - const v8CoverageEnabled = process.env.COVERAGE_DIR && process.env.NODE_V8_COVERAGE; - - // 0 indicates success - // so if it's anything then exit here since bazel only collects coverage on success - if (exitCode !== 0 || !v8CoverageEnabled) { - if (result.error) { - console.error(result.error); - } - process.exit(exitCode == null ? 1 : exitCode); - } - - const coverageDir = process.env.COVERAGE_DIR; - const outputFile = process.env.COVERAGE_OUTPUT_FILE; - const sourceFileManifest = process.env.COVERAGE_MANIFEST; - - const instrumentedSourceFiles = fs.readFileSync(sourceFileManifest).toString('utf8').split('\n'); - - // c8 will name the output report file lcov.info - // so we give it a dir that it can write to - // later on we'll move and rename it into output_file as bazel expects - const tmpdir = process.env['TEST_TMPDIR']; - const c8OutputDir = path.join(tmpdir, crypto.randomBytes(4).toString('hex')); - fs.mkdirSync(c8OutputDir); - - const includes = - instrumentedSourceFiles - // the manifest may include files such as .bash so we want to reduce that down to the set - // we can run coverage on in JS - .filter(f => ['.js', '.jsx', '.cjs', '.ts', '.tsx', '.mjs'].includes(path.extname(f))) - .map(f => { - // at runtime we only run .js or .mjs - // meaning that the coverage written by v8 will only include urls to .js or .mjs - // so the source files need to be mapped from their input to output extensions - // TODO: how do we know what source files produce .mjs and .cjs? - const p = path.parse(f); - let targetExt; - switch (p) { - case '.mjs': - targetExt = '.mjs'; - default: - targetExt = '.js'; - } - - return path.format({...p, base: null, ext: targetExt}); - }); - - // only require in c8 when we're actually going to do some coverage - const c8 = require('c8'); - // see https://github.com/bcoe/c8/blob/master/lib/report.js - // for more info on this function - // TODO: enable the --all functionality - await new c8 - .Report({ - include: includes, - // the test-exclude lib will include everything if out includes array is empty - // so instead when it's empty exclude everything - exclude: includes.length === 0 ? ['**'] : [], - reportsDirectory: c8OutputDir, - // tempDirectory as actually the dir that c8 will read from for the v8 json files - tempDirectory: coverageDir, - resolve: '', - // TODO: maybe add an attribute to allow more reporters - // or maybe an env var? - reporter: ['lcovonly'] - }) - .run(); - // moves the report into the files bazel expects - // lcovonly names this file lcov.info - fs.copyFileSync(path.join(c8OutputDir, 'lcov.info'), outputFile); - - process.exit(exitCode); -} - -if (require.main === module) { - process.exitCode = main(process.argv.slice(2)); -} diff --git a/packages/jasmine/src/index.from_src.bzl b/packages/jasmine/src/index.from_src.bzl index 6917c79aec..1b5ea72cf3 100644 --- a/packages/jasmine/src/index.from_src.bzl +++ b/packages/jasmine/src/index.from_src.bzl @@ -28,6 +28,5 @@ def jasmine_node_test( deps = deps + jasmine_deps, jasmine = "@npm_bazel_jasmine//:jasmine__pkg", jasmine_entry_point = "@npm_bazel_jasmine//:jasmine_runner.js", - coverage_entry_point = "@npm_bazel_jasmine//:coverage_runner.js", **kwargs ) diff --git a/packages/jasmine/src/jasmine_node_test.bzl b/packages/jasmine/src/jasmine_node_test.bzl index d22874d913..4f07de1d75 100644 --- a/packages/jasmine/src/jasmine_node_test.bzl +++ b/packages/jasmine/src/jasmine_node_test.bzl @@ -94,7 +94,7 @@ def jasmine_node_test( **kwargs: Remaining arguments are passed to the test rule """ if kwargs.pop("coverage", False): - fail("The coverage attribute has been removed, run your target with bazel coverage instead") + fail("The coverage attribute has been removed, run your target with \"bazel coverage\" instead") _devmode_js_sources( name = "%s_devmode_srcs" % name, diff --git a/packages/typescript/src/internal/build_defs.bzl b/packages/typescript/src/internal/build_defs.bzl index c39b20dc4a..c86ca69f1c 100644 --- a/packages/typescript/src/internal/build_defs.bzl +++ b/packages/typescript/src/internal/build_defs.bzl @@ -301,10 +301,6 @@ def _ts_library_impl(ctx): # TODO: Add remaining shared JS provider from design doc # (JSModuleInfo) and remove legacy "typescript" provider # once it is no longer needed. - - # indicates that the files listed in this invocation under srcs should be instrumented by coverage - # also transitively collect from deps - coverage_common.instrumented_files_info(ctx, source_attributes = ["srcs"], dependency_attributes = ["deps"]), ]) if ctx.attr.module_name: