Skip to content

test: increase timeout in break-on-uncaught - #10822

Closed
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:debugger-break-on-uncaught
Closed

test: increase timeout in break-on-uncaught#10822
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:debugger-break-on-uncaught

Conversation

@thefourtheye

Copy link
Copy Markdown
Contributor

As the failures suggest, this test expects the uncaught exception to
be thrown within 100 milliseconds, but on some of the test machines it
takes longer than that limit to notify the exception. Thats why the
test was failing.

This patch increases the timeout to three seconds, with platform
specific timeout value.

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

test


Refer: #10456

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jan 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/cc @nodejs/testing

Might it be better to use a short setInterval() (or a setTimeout() that renews itself if the thing hasn't paused yet) and let the test time out via test.py rather than having an AssertionError here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(That will also have the benefit of not requiring a minimum of 6 seconds for the test to run.)

@TrottTrottJan 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe like this?:

...
letinterval;functionrunTest(client){client.req({command: 'setexceptionbreak',arguments: {type: 'uncaught',enabled: true}},function(error){assert.ifError(error);client.on('exception',function(event){exceptions.push(event.body);});client.reqContinue(function(error){assert.ifError(error);interval=setInterval(assertHasPaused.bind(null,client),1);});});}functionassertHasPaused(client){if(exceptions.length>0){assert.strictEqual(exceptions.length,1,'debugger did not pause on exception');assert.strictEqual(exceptions[0].uncaught,true);assert.strictEqual(exceptions[0].script.name,testScript);assert.strictEqual(exceptions[0].sourceLine+1,throwsOnLine);asserted=true;client.reqContinue(assert.ifError);clearInterval(interval);}}}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It makes sense. Tested locally as well. Thanks :-)

@thefourtheye
thefourtheyeforce-pushed the debugger-break-on-uncaught branch from 33d556f to a3c8ffdCompareJanuary 16, 2017 19:14
@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

@Trott Does this LGTY?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I'm not mistaken, it's impossible for this assertion to fire given the check two lines above. So this line can probably be removed?

@TrottTrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a nit question/comment, but either way, LGTM if linter is green.

As the failures suggest, this test expects the uncaught exception to
be thrown within 100 milliseconds, but on some of the test machines it
takes longer than that limit to notify the exception. Thats why the
test was failing.
This patch polls every 10 ms to see if the exception is received.
@thefourtheye
thefourtheyeforce-pushed the debugger-break-on-uncaught branch from a3c8ffd to bac94e6CompareJanuary 28, 2017 08:01
@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

@Trott I updated the PR with your suggestion. I'll land this today, if you have no objections.

@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

Landed in cc899a4

@thefourtheye
thefourtheye deleted the debugger-break-on-uncaught branch January 30, 2017 12:44
thefourtheye added a commit that referenced this pull request Jan 30, 2017
As the failures suggest, this test expects the uncaught exception to
be thrown within 100 milliseconds, but on some of the test machines it
takes longer than that limit to notify the exception. Thats why the
test was failing.
This patch polls every 10 ms to see if the exception is received.
PR-URL: #10822
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@thefourtheyethefourtheye mentioned this pull request Jan 30, 2017
2 tasks
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
As the failures suggest, this test expects the uncaught exception to
be thrown within 100 milliseconds, but on some of the test machines it
takes longer than that limit to notify the exception. Thats why the
test was failing.
This patch polls every 10 ms to see if the exception is received.
PR-URL: #10822
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@italoacasasitaloacasas mentioned this pull request Jan 31, 2017
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Feb 5, 2017
As the failures suggest, this test expects the uncaught exception to
be thrown within 100 milliseconds, but on some of the test machines it
takes longer than that limit to notify the exception. Thats why the
test was failing.
This patch polls every 10 ms to see if the exception is received.
PR-URL: nodejs#10822
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@thefourtheyethefourtheye mentioned this pull request Feb 5, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
As the failures suggest, this test expects the uncaught exception to
be thrown within 100 milliseconds, but on some of the test machines it
takes longer than that limit to notify the exception. Thats why the
test was failing.
This patch polls every 10 ms to see if the exception is received.
PR-URL: #10822
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
As the failures suggest, this test expects the uncaught exception to
be thrown within 100 milliseconds, but on some of the test machines it
takes longer than that limit to notify the exception. Thats why the
test was failing.
This patch polls every 10 ms to see if the exception is received.
PR-URL: #10822
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 9, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@thefourtheye@Trott@santigimeno@mscdex@MylesBorins@nodejs-github-bot