fixup! feat(builtin): enable coverage on nodejs_test

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")
 

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();
@@ -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

internal/coverage/lcov_merger.ts

@@ -19,24 +19,23 @@ 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) {
@@ -74,7 +73,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

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}
 
@@ -222,13 +220,40 @@ export BAZEL_PATCH_ROOT=$(dirname $PWD)
 # 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"
+    fi
+    export NODE_V8_COVERAGE=${COVERAGE_DIR}
+  fi
+
+  set +e
+  "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"}
+  RESULT="$?"
+  set -e
+
+  if [ ${RESULT} -ne 0 ]; then
+    exit ${RESULT}
+  fi
+
+  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
+  fi
+  exit 0
 fi
 
 set +e

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]
@@ -226,6 +219,7 @@ def _nodejs_binary_impl(ctx):
         ]),
         "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),
@@ -461,6 +455,10 @@ jasmine_node_test(
         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,

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",
@@ -54,7 +53,6 @@ filegroup(
 js_library(
     name = "jasmine__pkg",
     srcs = [
-        "coverage_runner.js",
         "index.js",
         "jasmine_runner.js",
     ],

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
     )

packages/jasmine/src/jasmine_node_test.bzl

@@ -31,7 +31,6 @@ def jasmine_node_test(
         config_file = None,
         jasmine = "@npm//@bazel/jasmine",
         jasmine_entry_point = "@npm//:node_modules/@bazel/jasmine/jasmine_runner.js",
-        coverage_entry_point = "@npm//:node_modules/@bazel/jasmine/coverage_runner.js",
         **kwargs):
     """Runs tests in NodeJS using the Jasmine test runner.
 
@@ -93,7 +92,7 @@ def jasmine_node_test(
     nodejs_test(
         name = name,
         data = all_data,
-        entry_point = coverage_entry_point,
+        entry_point = jasmine_entry_point,
         templated_args = templated_args,
         testonly = 1,
         expected_exit_code = expected_exit_code,