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

test: fix timing sensitivity in debugger test #11008

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@ofrobots
Contributor

ofrobots commented Jan 26, 2017

test-debugger-util-regression.js was sensitive to timing, which seems to have changed enough with V8 5.7 to cause this test to fail. Fix the test to ensure we take debugger steps only at stable states instead of erroneously taking a step on a partially complete buffer.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@addaleax

LGTM if CI is happy

Show outdated Hide outdated test/parallel/test-debugger-util-regression.js
@@ -4,6 +4,8 @@ const path = require('path');
const spawn = require('child_process').spawn;
const assert = require('assert');
const DELAY = 200; //ms.

This comment has been minimized.

@addaleax

addaleax Jan 26, 2017

Member

Is it worth wrapping this in common.platformTimeout?

@addaleax

addaleax Jan 26, 2017

Member

Is it worth wrapping this in common.platformTimeout?

This comment has been minimized.

@ofrobots

ofrobots Jan 26, 2017

Contributor

Yep. Good idea.

@ofrobots

ofrobots Jan 26, 2017

Contributor

Yep. Good idea.

@@ -21,12 +23,16 @@ proc.stderr.setEncoding('utf8');
let stdout = '';
let stderr = '';
proc.stdout.on('data', (data) => stdout += data);
proc.stderr.on('data', (data) => stderr += data);

This comment has been minimized.

@addaleax

addaleax Jan 26, 2017

Member

I think proc.stdout.setEncoding('utf-8') would be good practice here (and the same for proc.stderr)

@addaleax

addaleax Jan 26, 2017

Member

I think proc.stdout.setEncoding('utf-8') would be good practice here (and the same for proc.stderr)

This comment has been minimized.

@ofrobots

ofrobots Jan 26, 2017

Contributor

That's already there, a few lines above, unchanged by my patch.

@ofrobots

ofrobots Jan 26, 2017

Contributor

That's already there, a few lines above, unchanged by my patch.

@mscdex mscdex added the debugger label Jan 26, 2017

test: fix timing sensitivity in debugger test
test-debugger-util-regression.js was sensitive to timing, which seems
to have changed enough with V8 5.7 to cause this test to fail. Fix the
test to ensure we take debugger steps only at stable states instead of
erroneously taking a step on a partially complete buffer.

PR-URL: #11008
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>

jasnell added a commit that referenced this pull request Feb 2, 2017

test: fix timing sensitivity in debugger test
test-debugger-util-regression.js was sensitive to timing, which seems
to have changed enough with V8 5.7 to cause this test to fail. Fix the
test to ensure we take debugger steps only at stable states instead of
erroneously taking a step on a partially complete buffer.

PR-URL: #11008
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 2, 2017

Member

Landed in 21b05cd

Member

jasnell commented Feb 2, 2017

Landed in 21b05cd

@jasnell jasnell closed this Feb 2, 2017

italoacasas added a commit that referenced this pull request Feb 3, 2017

test: fix timing sensitivity in debugger test
test-debugger-util-regression.js was sensitive to timing, which seems
to have changed enough with V8 5.7 to cause this test to fail. Fix the
test to ensure we take debugger steps only at stable states instead of
erroneously taking a step on a partially complete buffer.

PR-URL: #11008
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: fix timing sensitivity in debugger test
test-debugger-util-regression.js was sensitive to timing, which seems
to have changed enough with V8 5.7 to cause this test to fail. Fix the
test to ensure we take debugger steps only at stable states instead of
erroneously taking a step on a partially complete buffer.

PR-URL: nodejs#11008
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: fix timing sensitivity in debugger test
test-debugger-util-regression.js was sensitive to timing, which seems
to have changed enough with V8 5.7 to cause this test to fail. Fix the
test to ensure we take debugger steps only at stable states instead of
erroneously taking a step on a partially complete buffer.

PR-URL: nodejs#11008
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

test: fix timing sensitivity in debugger test
test-debugger-util-regression.js was sensitive to timing, which seems
to have changed enough with V8 5.7 to cause this test to fail. Fix the
test to ensure we take debugger steps only at stable states instead of
erroneously taking a step on a partially complete buffer.

PR-URL: #11008
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 9, 2017

test: fix timing sensitivity in debugger test
test-debugger-util-regression.js was sensitive to timing, which seems
to have changed enough with V8 5.7 to cause this test to fail. Fix the
test to ensure we take debugger steps only at stable states instead of
erroneously taking a step on a partially complete buffer.

PR-URL: #11008
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment