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

move cov to nodejs test #1135

Merged
merged 1 commit into from
May 5, 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
1 change: 1 addition & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ pkg_npm(
"//internal/bazel_integration_test:package_contents",
"//internal/common:package_contents",
"//internal/copy_repository:package_contents",
"//internal/coverage:package_contents",
"//internal/golden_file_test:package_contents",
"//internal/js_library:package_contents",
"//internal/linker:package_contents",
Expand Down
4 changes: 4 additions & 0 deletions common.bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ common --experimental_allow_incremental_repository_updates
build --incompatible_strict_action_env
run --incompatible_strict_action_env

# 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.
# .bazelrc.user should appear in .gitignore so that settings are not shared with team members
# This needs to be last statement in this
Expand Down
9 changes: 0 additions & 9 deletions e2e/jasmine/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,3 @@ jasmine_node_test(
"@npm//zone.js",
],
)

jasmine_node_test(
name = "coverage_test",
srcs = [
"coverage.spec.js",
"coverage_source.js",
],
coverage = True,
)
10 changes: 0 additions & 10 deletions e2e/jasmine/coverage.spec.js

This file was deleted.

40 changes: 40 additions & 0 deletions internal/coverage/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")

exports_files([
"lcov_merger-js.js",
])

# BEGIN-INTERNAL
load("//packages/typescript:checked_in_ts_project.bzl", "checked_in_ts_project")

checked_in_ts_project(
name = "lcov_merger_js_lib",
src = "lcov_merger.ts",
checked_in_js = "lcov_merger-js.js",
visibility = ["//visibility:public"],
deps = ["@npm//@types/node"],
)
# END-INTERNAL

nodejs_binary(
name = "lcov_merger_js",
entry_point = "lcov_merger.js",
visibility = ["//visibility:public"],
)

sh_binary(
name = "lcov_merger_sh",
srcs = ["lcov_merger.sh"],
visibility = ["//visibility:public"],
)

filegroup(
name = "package_contents",
srcs = glob([
"*.sh",
"*.js",
]) + [
"BUILD.bazel",
],
visibility = ["//:__pkg__"],
)
70 changes: 70 additions & 0 deletions internal/coverage/lcov_merger-js.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/* THIS FILE GENERATED FROM .ts; see BUILD.bazel */ /* clang-format off */"use strict";
var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
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) : adopt(result.value).then(fulfilled, rejected); }
step((generator = generator.apply(thisArg, _arguments || [])).next());
});
};
Object.defineProperty(exports, "__esModule", { value: true });
const crypto = require("crypto");
const fs = require("fs");
const path = require("path");
function _getArg(argv, key) {
return argv.find(a => a.startsWith(key)).split('=')[1];
}
function main() {
return __awaiter(this, void 0, void 0, function* () {
const argv = process.argv;
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');
const c8OutputDir = path.join(tmpdir, crypto.randomBytes(4).toString('hex'));
fs.mkdirSync(c8OutputDir);
const includes = instrumentedSourceFiles
.filter(f => ['.js', '.jsx', '.cjs', '.ts', '.tsx', '.mjs'].includes(path.extname(f)))
.map(f => {
const p = path.parse(f);
let targetExt;
switch (p.ext) {
case '.mjs':
targetExt = '.mjs';
default:
targetExt = '.js';
}
return path.format(Object.assign(Object.assign({}, p), { base: undefined, ext: targetExt }));
});
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;
}
yield new c8
.Report({
include: includes,
exclude: includes.length === 0 ? ['**'] : [],
reportsDirectory: c8OutputDir,
tempDirectory: coverageDir,
resolve: '',
reporter: ['lcovonly']
})
.run();
fs.copyFileSync(path.join(c8OutputDir, 'lcov.info'), outputFile);
});
}
if (require.main === module) {
main();
}
24 changes: 24 additions & 0 deletions internal/coverage/lcov_merger.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#!/usr/bin/env bash

# @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.

# This is a workaround for bazelbuild/bazel#6293. Since Bazel 0.18.0, Bazel
# expects tests to have an "$lcov_merger' or "_lcov_merger" attribute that
# points to an executable. If this is missing, the test driver fails.

# Copied from https://github.com/bazelbuild/rules_go/blob/64c97b54ea2918fc7f1a59d68cd27d1fdb0bd663/go/tools/builders/lcov_merger.sh

exit 0
112 changes: 112 additions & 0 deletions internal/coverage/lcov_merger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/**
* @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';

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.
*/
async function main() {
// 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 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
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 or .cjs?
const p = path.parse(f);
let targetExt;
switch (p.ext) {
case '.mjs':
targetExt = '.mjs';
default:
targetExt = '.js';
}

return path.format({...p, base: undefined, ext: targetExt});
});

// only require in c8 when we're actually going to do some coverage
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
await new c8
.Report({
include: includes,
// the test-exclude lib will include everything if our includes array is empty
// so instead when it's empty exclude everything
// but when it does have a value, we only want to use those includes, so don't exclude
// anything
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);
}

if (require.main === module) {
main();
}
73 changes: 55 additions & 18 deletions internal/node/launcher.sh
Original file line number Diff line number Diff line change
Expand Up @@ -158,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}

Expand Down Expand Up @@ -254,31 +255,67 @@ fi
# 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[@]}"}
Copy link
Author

Choose a reason for hiding this comment

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

by not using exec do we still handle "stdin, stdout, signals, etc" still?

Copy link
Author

Choose a reason for hiding this comment

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

also if we're ditching exec could we compbine this with the one belonw, Line 260

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. I think combining the two is reasonable. I don't think there is much value in running coverage when expected exit code != 0 but maybe someone will find that useful and it would simplify the code

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point re: the exec. We should look back at why the exec was there (trace back to the commit where it was added and see what it was fixing as I don't recall) and if there is anything special needed to pipe the io streams & signals. The non-exec non-zero exit code path had no special handling but that path is likely not used at all outside of this repo.

Copy link
Author

Choose a reason for hiding this comment

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

This looks like where it was introduced c124496

Copy link
Author

Choose a reason for hiding this comment

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

Looks like w'ell need to go back to the trap approah to handle SIGTERM, SIGINT, SIGQUIT. will throw that together and see what it looks like with that other refactor in

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @Toxicable . Was just looking at that commit and came to the same conclusion.

Copy link
Author

Choose a reason for hiding this comment

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

Modified it based off your changes and the ones in that MR, please check it well, my bash skills are not amazing

# 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 &
readonly 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}
Loading