Skip to content

test: changed test1 of test-vm-timeout.js so that entire error message would be matched - #11590

Closed
moe-dizzle wants to merge 3 commits into
nodejs:masterfrom
moe-dizzle:master
Closed

test: changed test1 of test-vm-timeout.js so that entire error message would be matched#11590
moe-dizzle wants to merge 3 commits into
nodejs:masterfrom
moe-dizzle:master

Conversation

@moe-dizzle

@moe-dizzlemoe-dizzle commented Feb 27, 2017

Copy link
Copy Markdown
Contributor

Before test 1 of test-vm-timeout.js would match any error, now it looks specifically for the error message "Script execution timed out."

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@moe-dizzle
test: changed test1 of test-vm-timeout.js so that entire error messag…
50fe4f0
…e would be matached in assert.throw
@moe-dizzle
test: changed test1 of test-vm-timeout.js so that entire error messag…
9fad978
…e would be matched
Before test 1 of test-vm-timeout.js would match any error,
now it looks specifically for the error message "Script execution timed out."
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Feb 27, 2017
@mscdexmscdex added the vm Issues and PRs related to the vm subsystem. label Feb 27, 2017
mscdex
mscdex reviewed Feb 27, 2017
View reviewed changes
Comment threadtest/parallel/test-vm-timeout.js Outdated
// Test 1: Timeout of 100ms executing endless loop
assert.throws(function() {
vm.runInThisContext('while(true) {}', { timeout: 100 });
}, function(err) {

@mscdexmscdexFeb 27, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think using a regexp as the second argument would be more succinct: /^Error: Script execution timed out\.$/

@moe-dizzlemoe-dizzleFeb 27, 2017

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.

Haha I agree with you before when I tried /^Script execution timed out.$/ it didn't work, so I did the above approached. I didn't realize that "Error: " was included also. Awesome I will correct it!

Trott
Trott approved these changes Feb 27, 2017
View reviewed changes

@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.

LGTM

@Trott

Trott commented Feb 27, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/6615/

@mscdex

mscdex commented Feb 27, 2017

Copy link
Copy Markdown
Contributor

LGTM

@addaleax

addaleax commented Mar 5, 2017

Copy link
Copy Markdown
Member

Landed in 94d1c8d, thanks for the PR!

@addaleaxaddaleax closed this Mar 5, 2017
addaleax pushed a commit that referenced this pull request Mar 5, 2017
@moe-dizzle@addaleax
test: changed test1 of test-vm-timeout.js
94d1c8d
test: changed test1 of test-vm-timeout.js so that entire error message
would be matched in assert.throw.
Before test 1 of test-vm-timeout.js would match any error,
now it looks specifically for the error message
"Script execution timed out."
PR-URL: #11590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 5, 2017
@moe-dizzle@addaleax
test: changed test1 of test-vm-timeout.js
b4dcb26
test: changed test1 of test-vm-timeout.js so that entire error message
would be matched in assert.throw.
Before test 1 of test-vm-timeout.js would match any error,
now it looks specifically for the error message
"Script execution timed out."
PR-URL: #11590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucasevanlucas mentioned this pull request Mar 8, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@moe-dizzle@MylesBorins
test: changed test1 of test-vm-timeout.js
9ecc2c9
test: changed test1 of test-vm-timeout.js so that entire error message
would be matched in assert.throw.
Before test 1 of test-vm-timeout.js would match any error,
now it looks specifically for the error message
"Script execution timed out."
PR-URL: #11590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@moe-dizzle@MylesBorins
test: changed test1 of test-vm-timeout.js
a43aa0e
test: changed test1 of test-vm-timeout.js so that entire error message
would be matched in assert.throw.
Before test 1 of test-vm-timeout.js would match any error,
now it looks specifically for the error message
"Script execution timed out."
PR-URL: #11590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@moe-dizzle@andrew749
test: changed test1 of test-vm-timeout.js
719c864
test: changed test1 of test-vm-timeout.js so that entire error message
would be matched in assert.throw.
Before test 1 of test-vm-timeout.js would match any error,
now it looks specifically for the error message
"Script execution timed out."
PR-URL: nodejs/node#11590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincalpinca approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@mscdexmscdexmscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

testIssues and PRs related to the tests.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@moe-dizzle@Trott@mscdex@addaleax@jasnell@lpinca@cjihrig@MylesBorins@nodejs-github-bot