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.

Sign up for GitHub

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

Jump to bottom

test: Fix flaky test-vm-timeout-rethrow #11530

Closed
wants to merge 3 commits into from
Closed

test: Fix flaky test-vm-timeout-rethrow #11530

wants to merge 3 commits into from

Conversation

kunalspathak
Copy link
Member

The intention of test case is to make sure that timeout property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before timeout for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: #11261

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

@kunalspathak
test: Fix flaky test-vm-timeout-rethrow
247d487 
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: nodejs#11261
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 24, 2017
addaleax
addaleax approved these changes Feb 24, 2017
View reviewed changes
test/sequential/test-vm-timeout-rethrow.js Outdated
@@ -6,7 +6,7 @@ const spawn = require('child_process').spawn;

if (process.argv[2] === 'child') {
const code = 'let j = 0;\n' +
'for (let i = 0; i < 1000000; i++) j += add(i, i + 1);\n' +
'while(true);\n' +
'j;';

const ctx = vm.createContext({
Copy link
Member

Choose a reason for hiding this comment

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

Can you drop theadd() function here as well?

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Feb 24, 2017
joshgav
joshgav approved these changes Feb 24, 2017
View reviewed changes
Copy link
Contributor

@joshgav joshgav left a comment

Choose a reason for hiding this comment

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

LGTM with a nit. Thanks Kunal!

test/sequential/test-vm-timeout-rethrow.js Outdated
@@ -6,14 +6,10 @@ const spawn = require('child_process').spawn;

if (process.argv[2] === 'child') {
const code = 'let j = 0;\n' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reduce the whole block to while(true); while you're at it? I.e. no need for declaring j either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it intentionally so it can be used in return, but with while(true); won't be necessary. Will remove it.

@joshgav
Copy link
Contributor

joshgav commented Feb 24, 2017

Landed in c3bc48f. Thanks!

@joshgav joshgav closed this Feb 24, 2017
joshgav pushed a commit that referenced this pull request Feb 24, 2017
@kunalspathak @joshgav
test: fix flaky test-vm-timeout-rethrow
c3bc48f 
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: #11261
PR-URL: #11530
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Josh Gavant <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
@kunalspathak @italoacasas
test: fix flaky test-vm-timeout-rethrow
ff85483 
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: nodejs#11261
PR-URL: nodejs#11530
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Josh Gavant <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@kunalspathak @jasnell
test: fix flaky test-vm-timeout-rethrow
60cde96 
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: #11261
PR-URL: #11530
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Josh Gavant <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@kunalspathak @jasnell
test: fix flaky test-vm-timeout-rethrow
d481996 
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: #11261
PR-URL: #11530
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Josh Gavant <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@kunalspathak @MylesBorins
test: fix flaky test-vm-timeout-rethrow
05909d0 
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: #11261
PR-URL: #11530
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Josh Gavant <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@kunalspathak @MylesBorins
test: fix flaky test-vm-timeout-rethrow
f0c7c7f 
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: #11261
PR-URL: #11530
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Josh Gavant <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Mar 12, 2017 •
edited
Loading

@kunalspathak FWIW: Got my testing stuck today on this test on Windows 7 x64. Process explorer showed node process suspended. I had commanded to resume the node process and the test ended successfully. However, I could not reproduce this afterward by running this test separately.

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

@joshgav joshgav joshgav approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-vm-timeout-rethrow on Windows
8 participants
@kunalspathak @joshgav @vsemozhetbyt @jasnell @addaleax @cjihrig @mscdex @nodejs-github-bot