Skip to content

Commit

Permalink
test: exclude tests only Node 12.6.0, not for >= 12.6.0
Browse files Browse the repository at this point in the history
The issue should be fixed in the next Node release, see
nodejs/node#28562.

Also, log a warning when Node.js v12.6.0 is used and uncaught exception
reporting is enabled.
  • Loading branch information
basti1302 committed Jul 7, 2019
1 parent 7c39228 commit 0857b54
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 4 deletions.
9 changes: 9 additions & 0 deletions packages/collector/src/uncaught/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ function activateUncaughtExceptionHandling() {
try {
if (require.resolve('netlinkwrapper')) {
logger.info('Reporting uncaught exceptions is enabled.');
if (process.version === 'v12.6.0') {
logger.warn(
'You are running Node.js v12.6.0 and have enabled reporting uncaught exceptions. To enable this feature, ' +
'@instana/collector will register an uncaughtException handler. Due to a bug in Node.js v12.6.0, the ' +
'original stack trace will get lost when this process is terminated with an uncaught exception. ' +
'Instana recommends to use a different Node.js version (<= v12.5.0 or >= v12.6.1). See ' +
'https://github.com/nodejs/node/issues/28550 for details.'
);
}
} else {
// This should actually not happen as require.resolve should either return a resolved filename or throw an
// exception.
Expand Down
10 changes: 6 additions & 4 deletions packages/collector/test/uncaught/assumptions_test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const expect = require('chai').expect;
const semver = require('semver');

const controls = require('./apps/controls');

Expand All @@ -27,7 +26,9 @@ describe('uncaught exceptions assumptions - ', () => {
expect(result.status).to.equal(7);
});

const keepStackTraceTest = semver.lt(process.version, '12.6.0') ? it.bind(this) : it.skip.bind(this);
// See https://github.com/nodejs/node/issues/28550 for why this test is excluded in v12.6.0 and
// https://github.com/nodejs/node/pull/28562/commits/36fdf1aa6c87ccfaebabb8f9c8004baab0549b0b for the fix.
const keepStackTraceTest = process.version !== 'v12.6.0' ? it.bind(this) : it.skip.bind(this);

keepStackTraceTest('keeps the original stack trace', () => {
const result = controls.rethrow();
Expand All @@ -48,8 +49,9 @@ describe('uncaught exceptions assumptions - ', () => {
const stdOut = result.stdout.toString('utf-8');
const stdErr = result.stderr.toString('utf-8');
expect(stdOut).to.match(/HANDLER 1\s*HANDLER 2\s*HANDLER 3/);
if (semver.lt(process.version, '12.6.0')) {
// see https://github.com/nodejs/node/issues/28550
if (process.version !== 'v12.6.0') {
// See https://github.com/nodejs/node/issues/28550 for why this test is excluded in v12.6.0 and
// https://github.com/nodejs/node/pull/28562/commits/36fdf1aa6c87ccfaebabb8f9c8004baab0549b0b for the fix.
expect(stdErr).to.not.be.empty;
expect(stdErr).to.contain('Error: Boom');
expect(stdErr).to.contain('_onTimeout');
Expand Down

0 comments on commit 0857b54

Please sign in to comment.