fixup! feat(builtin): enable coverage on nodejs_test

common.bazelrc

@@ -76,7 +76,7 @@ common --experimental_allow_incremental_repository_updates
 build --incompatible_strict_action_env
 run --incompatible_strict_action_env
 
-# coverage --test_env=LCOV_MERGER=/bin/true
+# when running `bazel coverage` ensure that the test targets are instrumented 
 coverage --instrument_test_targets
 
 # Load any settings specific to the current user.

internal/coverage/BUILD.bazel

@@ -5,11 +5,12 @@ nodejs_binary(
     name = "lcov_merger",
     data = [
         ":lcov_merger.js",
+        # TODO: what sort of error does a user get when they don't install c8
+        # we may need to do a check elsewhere to ensure it's installed if they want to use `bazel coverage`
         "@npm//c8",
     ],
     entry_point = ":lcov_merger.js",
     visibility = ["//visibility:public"],
-    # templated_args = ["--nobazel_patch_module_resolver"],
 )
 
 # BEGIN-INTERNAL
@@ -19,6 +20,9 @@ checked_in_ts_library(
     srcs = ["lcov_merger.ts"],
     checked_in_js = "lcov_merger.js",
     visibility = ["//internal/node:__subpackages__"],
-    deps = ["@npm//@types/node"],
+    deps = [
+        "@npm//@types/node",
+        "@npm//c8",
+    ],
 )
 # END-INTERNAL

internal/coverage/lcov_merger.js

@@ -1,55 +1,100 @@
-
-// this is what prints out all the other stuff
-// https://github.com/bazelbuild/bazel/blob/b3763e9c003398d265470dcbc9d925990ee9a2b2/tools/test/collect_coverage.sh#L175-L181
-
-// this handles merging v8 reports from a single test suite and pushes them into bazels output file
-// to be picked up by a combined report later on
-
-const fs = require('fs');
-const path = require('path');
-const c8Report = require('c8/lib/report')
-const crypto = require('crypto');
-
-
-// called from
-// https://github.com/bazelbuild/bazel/blob/b3763e9c003398d265470dcbc9d925990ee9a2b2/tools/test/collect_coverage.sh#L175-L181
-
-
-function getArgValues(name, args) {
-  const values = args.filter(a => a.startsWith('--' + name)).map(v => v.split('=')[1]);
-  return values;
-}
-function getArgValue(name, args) {
-  return getArgValues(name, args)[0];
-}
-
-async function main() {
-  const args = process.argv.splice(2);
-
-  const coverageDir = getArgValue('coverage_dir', args);
-  const outputFile = getArgValue('output_file', args);
-  const sourceFileManifest = getArgValue('source_file_manifest', args);
-  const filters = getArgValues('filter_sources', args);
-
-  const sourceFiles = fs.readFileSync(sourceFileManifest).toString('utf8').split('\n');
-
-  const c8OutputDir = path.join(process.env.TEST_TMPDIR, crypto.randomBytes(4).toString('hex'));
-
-  fs.mkdirSync(c8OutputDir);
-
-  await c8Report({
-    include: sourceFiles,
-    exclude: filters,
-    reportsDirectory: c8OutputDir,
-    tempDirectory: coverageDir,
-    // resolve: ''resolve paths to alternate base directory''
-    reporter: 'lcov'
-  }).run();
-
-  const lcovReport = fs.readdirSync(c8OutputDir)[0];
-
-
-  fs.copyFileSync(path.join(c8OutputDir, lcovReport), outputFile)
-}
-
-main();
+/* THIS FILE GENERATED FROM .ts; see BUILD.bazel */ /* clang-format off */var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
+    return new (P || (P = Promise))(function (resolve, reject) {
+        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
+        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
+        function step(result) { result.done ? resolve(result.value) : new P(function (resolve) { resolve(result.value); }).then(fulfilled, rejected); }
+        step((generator = generator.apply(thisArg, _arguments || [])).next());
+    });
+};
+(function (factory) {
+    if (typeof module === "object" && typeof module.exports === "object") {
+        var v = factory(require, exports);
+        if (v !== undefined) module.exports = v;
+    }
+    else if (typeof define === "function" && define.amd) {
+        define("build_bazel_rules_nodejs/internal/coverage/lcov_merger", ["require", "exports", "fs", "path", "crypto"], factory);
+    }
+})(function (require, exports) {
+    "use strict";
+    Object.defineProperty(exports, "__esModule", { value: true });
+    /**
+     * @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");
+    // unfortunnately there's no types for this at the time
+    // change to an import if some become avilable
+    // see https://github.com/bcoe/c8/issues/195
+    const c8Report = require('c8/lib/report');
+    /**
+     * If there are multiple args with the same name, returns an array of all of them
+     */
+    function getArgValues(name, args) {
+        const values = args.filter(a => a.startsWith('--' + name)).map(v => v.split('=')[1]);
+        return values;
+    }
+    /**
+     * returns the first occurance of the argument by the provided name
+     */
+    function getArgValue(name, args) {
+        return getArgValues(name, args)[0];
+    }
+    /**
+     * This script is called from https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh#L175-L181
+     * when collecting coverage.
+     * It's designed to collect the coverage of one target, since in nodejs and using NODEJS_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
+     */
+    function main() {
+        return __awaiter(this, void 0, void 0, function* () {
+            const args = process.argv.splice(2);
+            // TODO: validate these args exist
+            // otherwise we'll get hard to debug path errors
+            const coverageDir = getArgValue('coverage_dir', args);
+            const outputFile = getArgValue('output_file', args);
+            const sourceFileManifest = getArgValue('source_file_manifest', args);
+            const filters = getArgValues('filter_sources', args);
+            const instrumentedSourceFiles = fs.readFileSync(sourceFileManifest).toString('utf8').split('\n');
+            // c8 will name the output report file something that bazel wont expect
+            // 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);
+            // see https://github.com/bcoe/c8/blob/master/lib/report.js
+            // for more info on this function
+            yield c8Report({
+                // TODO: check if these filters work here, otherwise apply them beforehand
+                include: instrumentedSourceFiles,
+                exclude: filters,
+                reportsDirectory: c8OutputDir,
+                // tempDirectory as actually the dir that c8 will read from for the v8 json files
+                tempDirectory: coverageDir,
+                // we may need to use resolve here to change the base dir, from the docs: "resolve paths to alternate base directory"
+                // resolve: ''
+                // TODO: check that lcov is correct here, we may need to do another conversion
+                reporter: 'lcov'
+            }).run();
+            // this assumes there's only one report from c8
+            const lcovReport = fs.readdirSync(c8OutputDir)[0];
+            // mopves the report into the fiels bazel expects
+            fs.copyFileSync(path.join(c8OutputDir, lcovReport), outputFile);
+        });
+    }
+    main();
+});

internal/coverage/lcov_merger.ts

@@ -1,4 +1,92 @@
-console.log(process.argv)
+/**
+ * @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.
+ */
+import * as crypto from 'crypto';
+import * as fs from 'fs';
+import * as path from 'path';
 
-    // called from
-    // https://github.com/bazelbuild/bazel/blob/b3763e9c003398d265470dcbc9d925990ee9a2b2/tools/test/collect_coverage.sh#L175-L181
+// unfortunnately there's no types for this at the time
+// change to an import if some become avilable
+// see https://github.com/bcoe/c8/issues/195
+const c8Report = require('c8/lib/report');
+
+/**
+ * If there are multiple args with the same name, returns an array of all of them
+ */
+function getArgValues(name: string, args: string[]): string[] {
+  const values = args.filter(a => a.startsWith('--' + name)).map(v => v.split('=')[1]);
+  return values;
+}
+
+/**
+ * returns the first occurance of the argument by the provided name
+ */
+function getArgValue(name: string, args: string[]): string {
+  return getArgValues(name, args)[0];
+}
+
+/**
+ * This script is called from
+ * https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh#L175-L181 when
+ * collecting coverage. It's designed to collect the coverage of one target, since in nodejs and
+ * using NODEJS_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
+ */
+async function main(): Promise<void> {
+  const args = process.argv.splice(2);
+
+  // TODO: validate these args exist
+  // otherwise we'll get hard to debug path errors
+  const coverageDir = getArgValue('coverage_dir', args);
+  const outputFile = getArgValue('output_file', args);
+  const sourceFileManifest = getArgValue('source_file_manifest', args);
+  const filters = getArgValues('filter_sources', args);
+
+  const instrumentedSourceFiles = fs.readFileSync(sourceFileManifest).toString('utf8').split('\n');
+
+  // c8 will name the output report file something that bazel wont expect
+  // 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'] as string;
+  const c8OutputDir = path.join(tmpdir, crypto.randomBytes(4).toString('hex'));
+  fs.mkdirSync(c8OutputDir);
+
+  // see https://github.com/bcoe/c8/blob/master/lib/report.js
+  // for more info on this function
+  await c8Report({
+    // TODO: check if these filters work here, otherwise apply them beforehand
+    include: instrumentedSourceFiles,
+    exclude: filters,
+    reportsDirectory: c8OutputDir,
+    // tempDirectory as actually the dir that c8 will read from for the v8 json files
+    tempDirectory: coverageDir,
+    // we may need to use resolve here to change the base dir, from the docs: "resolve paths to
+    // alternate base directory"
+    // resolve: ''
+    // TODO: check that lcov is correct here, we may need to do another conversion
+    reporter: 'lcov'
+  }).run();
+
+  // this assumes there's only one report from c8
+  const lcovReport = fs.readdirSync(c8OutputDir)[0];
+
+
+  // mopves the report into the fiels bazel expects
+  fs.copyFileSync(path.join(c8OutputDir, lcovReport), outputFile)
+}
+
+main();

internal/node/launcher.sh

@@ -90,6 +90,8 @@ fi
 export RUNFILES
 # --- end RUNFILES initialization ---
 
+# TODO: remove
+# this is just here for debug
 set -x
 
 TEMPLATED_env_vars

internal/node/node.bzl

@@ -187,14 +187,9 @@ def _nodejs_binary_impl(ctx):
             env_vars += "export %s=\"%s\"\n" % (k, ctx.configuration.default_shell_env[k])
 
     # indicates that this was run with `bazel coverage`
-    # and that we should collect and store coverage
-    # TODO: store coverage in the location bazel tells us
-    for dep in ctx.attr.data:
-        if ctx.coverage_instrumented(dep):
-            print(dep.label)
+    # and that we have files to instrumented somewhere in the deps
     if ctx.configuration.coverage_enabled:
         if (ctx.coverage_instrumented() or any([ctx.coverage_instrumented(dep) for dep in ctx.attr.data])):
-            print("there are covered deps")
             env_vars += "export NODE_V8_COVERAGE=$COVERAGE_DIR\n"
 
     expected_exit_code = 0
@@ -285,9 +280,11 @@ def _nodejs_binary_impl(ctx):
             deps = depset([ctx.file.entry_point], transitive = [node_modules, sources]),
             pkgs = ctx.attr.data,
         ),
-
         # indicates that the this binary should be instrumented by coverage
-        coverage_common.instrumented_files_info(ctx, source_attributes = ["data"], extensions = ["js"]),
+        # see https://docs.bazel.build/versions/master/skylark/lib/coverage_common.html
+        # since this will be called from a nodejs_test, where the entrypoint is going to be the test file
+        # we shouldn't add the entrypoint as a attribute to collect here
+        coverage_common.instrumented_files_info(ctx, dependency_attributes = ["data"], extensions = ["js", "ts"]),
     ]
 
 _NODEJS_EXECUTABLE_ATTRS = {
@@ -527,6 +524,8 @@ nodejs_test = rule(
             doc = "The expected exit code for the test. Defaults to 0.",
             default = 0,
         ),
+        # we shouldn't define this in `nodejs_binary` since `lcov_merger` is a `nodejs_binary`
+        # and this will only be executed under `bazel coverage` which has to be a `nodejs_test`
         "_lcov_merger": attr.label(
             executable = True,
             default = Label("@build_bazel_rules_nodejs//internal/coverage:lcov_merger"),

packages/jasmine/src/jasmine_runner.js

@@ -1,6 +1,8 @@
 const fs = require('fs');
 const bazelJasmine = require('@bazel/jasmine');
 
+// TODO: remove
+// jsut here for debug purposes
 // console.log(process.env);
 // https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh
 console.log('COVERAGE_MANIFEST', process.env.COVERAGE_MANIFEST);

packages/typescript/src/internal/build_defs.bzl

@@ -287,7 +287,8 @@ def _ts_library_impl(ctx):
         # once it is no longer needed.,
 
         # indicates that the files listed in this invocation under srcs should be instrumented by coverage
-        coverage_common.instrumented_files_info(ctx, source_attributes = ["srcs"]),
+        # also transitively collect from deps
+        coverage_common.instrumented_files_info(ctx, source_attributes = ["srcs"], dependency_attributes = ["deps"]),
     ])
 
     return ts_providers_dict_to_struct(ts_providers)