fixup! feat(builtin): enable coverage on nodejs_test

bazel-contrib · Apr 8, 2020 · bdd7e76 · bdd7e76
1 parent 2e3fc08
commit bdd7e76
Show file tree

Hide file tree

Showing 11 changed files with 105 additions and 174 deletions.
diff --git 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
@@ -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
@@ -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
@@ -19,32 +19,30 @@ 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];
 }
 
 /**
  * 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
  */
 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) {
     throw new Error();
   }
 
   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
@@ -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}
diff --git 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
@@ -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",
     ],