From 60b59c455c3e4ee8f6392dcc1cec6fd056c97a61 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 10 May 2018 11:21:25 -0700 Subject: [PATCH 1/9] Add coverage reporting for JavaScript and TypeScript files --- gulpfile.ts | 2 +- package.json | 20 +++++++++++++++++--- run-tests.sh | 18 +++++++++++++++--- test/gulpfile.js | 20 ++++++++++++++------ 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/gulpfile.ts b/gulpfile.ts index 388a74f4..dac1421e 100644 --- a/gulpfile.ts +++ b/gulpfile.ts @@ -102,7 +102,7 @@ gulp.task('test.only', 'Run tests without rebuilding anything', ['js.core.test', 'native.test.only']); gulp.task('test', 'Run all tests', (callback) => { - runSequence('build', 'test.only', callback); + runSequence('build', 'test.only', 'internal.test.test', callback); }); gulp.task('doc.gen', 'Generate documentation', ['native.core.doc.gen']); diff --git a/package.json b/package.json index fb626214..053f7d29 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "@types/node": "^8.0.32", "@types/pify": "^3.0.0", "@types/semver": "^5.5.0", + "coveralls": "^3.0.1", "del": "^3.0.0", "execa": "^0.8.0", "gulp": "^3.9.1", @@ -35,8 +36,11 @@ "mocha": "^3.5.3", "mocha-jenkins-reporter": "^0.3.9", "ncp": "^2.0.0", + "nyc": "^11.7.2", "pify": "^3.0.0", + "run-sequence": "^2.2.0", "semver": "^5.5.0", + "symlink": "^2.1.0", "through2": "^2.0.3", "ts-node": "^3.3.0", "tslint": "^5.5.0", @@ -48,8 +52,18 @@ "name": "Google Inc." } ], - "dependencies": { - "run-sequence": "^2.2.0", - "symlink": "^2.1.0" + "nyc": { + "include": [ + "packages/grpc-health-check/health.js", + "packages/grpc-js-core/build/src/*", + "packages/grpc-native-core/index.js", + "packages/grpc-native-core/src/*.js", + "packages/grpc-protobufjs/build/src/*" + ], + "cache": true + }, + "scripts": { + "test": "nyc gulp test", + "coverage": "nyc report --reporter=text-lcov | coveralls" } } diff --git a/run-tests.sh b/run-tests.sh index 300a2527..afce89bb 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -39,6 +39,16 @@ npm install --unsafe-perm mkdir -p reports export JOBS=8 +export JUNIT_REPORT_PATH="reports/node$version/" +export JUNIT_REPORT_STACK=1 + +gsutil cp gs://grpc-testing-secrets/coveralls_credentials/grpc-node.rc . +set +x +. grpc-node.rc +set -x +export COVERALLS_REPO_TOKEN +export COVERALLS_SERVICE_NAME=Kokoro +export COVERALLS_SERVICE_JOB_ID=$KOKORO_BUILD_ID # TODO(mlumish): Add electron tests @@ -62,8 +72,8 @@ do ./node_modules/.bin/gulp clean.all ./node_modules/.bin/gulp setup - # Rebuild libraries and run tests. - JUNIT_REPORT_PATH="reports/node$version/" JUNIT_REPORT_STACK=1 ./node_modules/.bin/gulp test || FAILED="true" + # npm test calls nyc gulp test + npm test || FAILED="true" done set +ex @@ -72,7 +82,9 @@ set -ex node merge_kokoro_logs.js -if [ "$FAILED" != "" ] +if [ "$FAILED" == "" ] then + npm run coverage +else exit 1 fi diff --git a/test/gulpfile.js b/test/gulpfile.js index 61ff1658..b6221ed6 100644 --- a/test/gulpfile.js +++ b/test/gulpfile.js @@ -21,6 +21,7 @@ const mocha = require('gulp-mocha'); const execa = require('execa'); const path = require('path'); const del = require('del'); +const semver = require('semver'); const linkSync = require('../util').linkSync; // gulp-help monkeypatches tasks to have an additional description parameter @@ -51,12 +52,19 @@ gulp.task('test', 'Run API-level tests', () => { .on('end', resolve) .on('error', reject); }); - const runTestsArgPairs = [ - ['native', 'native'], - ['native', 'js'], - // ['js', 'native'], - // ['js', 'js'] - ]; + let runTestsArgPairs; + if (semver.satisfies(process.version, '>=9.4')) { + runTestsArgPairs = [ + ['native', 'native'], + ['native', 'js'], + // ['js', 'native'], + // ['js', 'js'] + ]; + } else { + runTestsArgPairs = [ + ['native', 'native'] + ]; + } return runTestsArgPairs.reduce((previousPromise, argPair) => { return previousPromise.then(runTestsWithFixture.bind(null, argPair[0], argPair[1])); }, Promise.resolve()); From 796a1df0e50c675822b057afa56ca03b07b209c3 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 10:59:19 -0700 Subject: [PATCH 2/9] Make interop client fail if the test never finishes --- package.json | 6 +++++- test/api/interop_sanity_test.js | 2 +- test/interop/interop_client.js | 8 ++++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 053f7d29..47dda40f 100644 --- a/package.json +++ b/package.json @@ -60,7 +60,11 @@ "packages/grpc-native-core/src/*.js", "packages/grpc-protobufjs/build/src/*" ], - "cache": true + "cache": true, + "all": true, + "require": [ + "ts-node/register" + ] }, "scripts": { "test": "nyc gulp test", diff --git a/test/api/interop_sanity_test.js b/test/api/interop_sanity_test.js index 1152b310..0ff4c28b 100644 --- a/test/api/interop_sanity_test.js +++ b/test/api/interop_sanity_test.js @@ -68,7 +68,7 @@ describe('Interop tests', function() { throw new Error(`Server exited with signal ${signal}`); } } - }) + }); }); after(function() { serverProcess.send({}); diff --git a/test/interop/interop_client.js b/test/interop/interop_client.js index fb6026e0..195ef8d4 100644 --- a/test/interop/interop_client.js +++ b/test/interop/interop_client.js @@ -616,8 +616,12 @@ if (require.main === module) { }; runTest(argv.server_host + ':' + argv.server_port, argv.server_host_override, argv.test_case, argv.use_tls === 'true', argv.use_test_ca === 'true', - function () { - console.log('OK:', argv.test_case); + function (err) { + if (err) { + throw err; + } else { + console.log('OK:', argv.test_case); + } }, extra_args); } From 09194cb757180938a31c9cd3a4eee92d3b4dff32 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 11:00:15 -0700 Subject: [PATCH 3/9] Reference and unreference http2 sessions more dynamically --- packages/grpc-js-core/src/channel.ts | 37 ++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/grpc-js-core/src/channel.ts b/packages/grpc-js-core/src/channel.ts index c03389d8..40b5a93d 100644 --- a/packages/grpc-js-core/src/channel.ts +++ b/packages/grpc-js-core/src/channel.ts @@ -82,6 +82,15 @@ export interface Channel extends EventEmitter { /* tslint:enable:no-any */ } +/* This should be a real subchannel class that contains a ClientHttp2Session, + * but for now this serves its purpose */ +type Http2SubChannel = http2.ClientHttp2Session & { + /* Count the number of currently active streams associated with the session. + * The purpose of this is to keep the session reffed if and only if there + * is at least one active stream */ + streamCount?: number; +}; + export class Http2Channel extends EventEmitter implements Channel { private readonly userAgent: string; private readonly target: url.URL; @@ -91,7 +100,7 @@ export class Http2Channel extends EventEmitter implements Channel { private connecting: Promise|null = null; /* For now, we have up to one subchannel, which will exist as long as we are * connecting or trying to connect */ - private subChannel: http2.ClientHttp2Session|null = null; + private subChannel: Http2SubChannel|null = null; private filterStackFactory: FilterStackFactory; private subChannelConnectCallback: () => void = () => {}; @@ -160,7 +169,7 @@ export class Http2Channel extends EventEmitter implements Channel { } private startConnecting(): void { - let subChannel: http2.ClientHttp2Session; + let subChannel: Http2SubChannel; const secureContext = this.credentials.getSecureContext(); if (secureContext === null) { subChannel = http2.connect(this.target); @@ -260,11 +269,25 @@ export class Http2Channel extends EventEmitter implements Channel { headers[HTTP2_HEADER_PATH] = methodName; headers[HTTP2_HEADER_TE] = 'trailers'; if (this.connectivityState === ConnectivityState.READY) { - const session: http2.ClientHttp2Session = this.subChannel!; - // Prevent the HTTP/2 session from keeping the process alive. - // Note: this function is only available in Node 9 - session.unref(); - stream.attachHttp2Stream(session.request(headers)); + const session: Http2SubChannel = this.subChannel!; + let http2Stream = session.request(headers); + /* This is a very ad-hoc reference counting scheme. This should be + * handled by a subchannel class */ + session.ref(); + if (!session.streamCount) { + session.streamCount = 0; + } + session.streamCount += 1; + http2Stream.on('close', () => { + if (!session.streamCount) { + session.streamCount = 0; + } + session.streamCount -= 1; + if (session.streamCount <= 0) { + session.unref(); + } + }); + stream.attachHttp2Stream(http2Stream); } else { /* In this case, we lost the connection while finalizing * metadata. That should be very unusual */ From 4b020981e0edcc07b9cdbe8584e7c8172d133faf Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 11:08:30 -0700 Subject: [PATCH 4/9] Fix minor issues with coveralls reporting script --- run-tests.sh | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/run-tests.sh b/run-tests.sh index afce89bb..03a45cfa 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -42,13 +42,18 @@ export JOBS=8 export JUNIT_REPORT_PATH="reports/node$version/" export JUNIT_REPORT_STACK=1 -gsutil cp gs://grpc-testing-secrets/coveralls_credentials/grpc-node.rc . -set +x -. grpc-node.rc -set -x -export COVERALLS_REPO_TOKEN -export COVERALLS_SERVICE_NAME=Kokoro -export COVERALLS_SERVICE_JOB_ID=$KOKORO_BUILD_ID +OS=$(uname) + +if [ "$OS" == "Linux" ] +then + gsutil cp gs://grpc-testing-secrets/coveralls_credentials/grpc-node.rc /tmp + set +x + . /tmp/grpc-node.rc + set -x + export COVERALLS_REPO_TOKEN + export COVERALLS_SERVICE_NAME=Kokoro + export COVERALLS_SERVICE_JOB_ID=$KOKORO_BUILD_ID +fi # TODO(mlumish): Add electron tests @@ -84,7 +89,10 @@ node merge_kokoro_logs.js if [ "$FAILED" == "" ] then - npm run coverage + if [ "$OS" == "Linux" ] + then + npm run coverage + fi else exit 1 fi From 5686bea87a71520598f0a3e17016b15cfebabe81 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 13:26:05 -0700 Subject: [PATCH 5/9] Remove possibly unneeded line from nyc config --- package.json | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/package.json b/package.json index 47dda40f..e8d7a112 100644 --- a/package.json +++ b/package.json @@ -61,10 +61,7 @@ "packages/grpc-protobufjs/build/src/*" ], "cache": true, - "all": true, - "require": [ - "ts-node/register" - ] + "all": true }, "scripts": { "test": "nyc gulp test", From 3ed2e806fd7bf397d2761c80521b0e257d97c0d3 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 14:03:11 -0700 Subject: [PATCH 6/9] Don't use let in js gulpfiles --- test/gulpfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/gulpfile.js b/test/gulpfile.js index b6221ed6..4c8cf64c 100644 --- a/test/gulpfile.js +++ b/test/gulpfile.js @@ -52,7 +52,7 @@ gulp.task('test', 'Run API-level tests', () => { .on('end', resolve) .on('error', reject); }); - let runTestsArgPairs; + var runTestsArgPairs; if (semver.satisfies(process.version, '>=9.4')) { runTestsArgPairs = [ ['native', 'native'], From fc51f0ea480a76b49f1f9c60ea1e7941513dee39 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 14:33:05 -0700 Subject: [PATCH 7/9] Some test script fixes --- run-tests.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/run-tests.sh b/run-tests.sh index 03a45cfa..7eac3814 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -39,7 +39,6 @@ npm install --unsafe-perm mkdir -p reports export JOBS=8 -export JUNIT_REPORT_PATH="reports/node$version/" export JUNIT_REPORT_STACK=1 OS=$(uname) @@ -57,6 +56,8 @@ fi # TODO(mlumish): Add electron tests +FAILED="false" + for version in ${node_versions} do # Install and setup node for the version we want. @@ -66,6 +67,8 @@ do nvm use $version set -ex + export JUNIT_REPORT_PATH="reports/node$version/" + # https://github.com/mapbox/node-pre-gyp/issues/362 npm install -g node-gyp @@ -87,12 +90,12 @@ set -ex node merge_kokoro_logs.js -if [ "$FAILED" == "" ] +if [ "$FAILED" == "true" ] then + exit 1 +else if [ "$OS" == "Linux" ] then npm run coverage fi -else - exit 1 fi From 1f58e0111d31ce90fa58a26d6d007e227edc07d0 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 16:03:57 -0700 Subject: [PATCH 8/9] Skip cross-implementation tests for Node<6 --- packages/grpc-protobufjs/package.json | 5 ++++- test/gulpfile.js | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/grpc-protobufjs/package.json b/packages/grpc-protobufjs/package.json index 1ed26975..8890edb8 100644 --- a/packages/grpc-protobufjs/package.json +++ b/packages/grpc-protobufjs/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/proto-loader", - "version": "0.1.0", + "version": "0.2.0", "author": "Google Inc.", "contributors": [ { @@ -47,5 +47,8 @@ "clang-format": "^1.2.2", "gts": "^0.5.3", "typescript": "~2.7.2" + }, + "engines": { + "node": ">=6" } } diff --git a/test/gulpfile.js b/test/gulpfile.js index 4c8cf64c..2aa1c9c2 100644 --- a/test/gulpfile.js +++ b/test/gulpfile.js @@ -39,6 +39,10 @@ gulp.task('clean.all', 'Delete all files created by tasks', () => {}); gulp.task('test', 'Run API-level tests', () => { // run mocha tests matching a glob with a pre-required fixture, // returning the associated gulp stream + if (!semver.satisfies(process.version, '>=9.4')) { + console.log(`Skipping cross-implementation tests for Node ${process.version}`); + return; + } const apiTestGlob = `${apiTestDir}/*.js`; const runTestsWithFixture = (server, client) => new Promise((resolve, reject) => { const fixture = `${server}_${client}`; From 908be509315f2b78cbe503fbc5fcb95582f49dae Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 14 May 2018 16:33:15 -0700 Subject: [PATCH 9/9] Fix string comparison in run-tests script --- run-tests.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/run-tests.sh b/run-tests.sh index 7eac3814..4484d40b 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -43,7 +43,7 @@ export JUNIT_REPORT_STACK=1 OS=$(uname) -if [ "$OS" == "Linux" ] +if [ "$OS" = "Linux" ] then gsutil cp gs://grpc-testing-secrets/coveralls_credentials/grpc-node.rc /tmp set +x @@ -90,11 +90,11 @@ set -ex node merge_kokoro_logs.js -if [ "$FAILED" == "true" ] +if [ "$FAILED" = "true" ] then exit 1 else - if [ "$OS" == "Linux" ] + if [ "$OS" = "Linux" ] then npm run coverage fi