FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: increase coverage of vm by DavidCai1111 · Pull Request #11377 · nodejs/node · GitHub

/ node Public

test: increase coverage of vm - #11377

Closed
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:test/vm-coverage
ConversationCommits (1)ChecksFiles changed
Closed

test: increase coverage of vm#11377
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:test/vm-coverage

Conversation

DavidCai1111 commented Feb 14, 2017

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 14, 2017
mscdex added the vm Issues and PRs related to the vm subsystem. label Feb 14, 2017
addaleax reviewed Feb 21, 2017
View reviewed changes
Comment thread test/parallel/test-vm-sigint-existing-handler.js Outdated

addaleax Feb 21, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: How about

const script = `process.send('${method}'); while(true) {}`;
const options = { breakOnSigint: true };
assert.throws(vm[method](script, ...args, options);

(with args kinda like above… you get the idea. 😄)

addaleax self-assigned this Feb 21, 2017
DavidCai1111 force-pushed the test/vm-coverage branch 2 times, most recently from c96ab60 to 1d6d58d Compare February 22, 2017 02:57

DavidCai1111 commented Feb 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@addaleax Updated, PTAL.

(Since the latest testing-guide says "for the ease of backporting, it is encouraged to use those ES.Next features that can be used directly without a flag in all maintained branches", so here i still use .apply instead of spread operator 😢 )

addaleax commented Feb 22, 2017

Copy link
Copy Markdown
Member

@DavidCai1993 This test doesn’t exist in v4.x and won’t ever be backported there, so you can feel completely free to use the spread operator. :)

DavidCai1111 commented Feb 22, 2017

Copy link
Copy Markdown
Member Author

@addaleax OK, updated 👍

addaleax approved these changes Feb 22, 2017
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks!

CI: https://ci.nodejs.org/job/node-test-commit/8073/

addaleax commented Feb 24, 2017

Copy link
Copy Markdown
Member

Landed in c7205ba, thanks for the PR!

addaleax closed this Feb 24, 2017
addaleax pushed a commit that referenced this pull request Feb 24, 2017
test: increase coverage of vm
c7205ba 
PR-URL: #11377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 24, 2017
test: increase coverage of vm
b395ed9 
PR-URL: #11377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas mentioned this pull request Feb 25, 2017
Merged

jasnell commented Mar 7, 2017

Copy link
Copy Markdown
Member

Does not land cleanly on v4.x due to missing files. Backport PR would be needed

jasnell pushed a commit that referenced this pull request Mar 7, 2017
test: increase coverage of vm
64afb0b 
PR-URL: #11377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
test: increase coverage of vm
6bf9366 
PR-URL: #11377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes

Assignees

addaleax

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.

7 participants

Back | FazBrowse Home | New Git URL