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.

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: increase coverage of vm #11377

Closed
wants to merge 1 commit into from

Conversation

DavidCai1111
Copy link
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 nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 14, 2017
@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Feb 14, 2017
}

assert.throws(() => { vm[method].apply(vm, args); },
/^Error: Script execution interrupted\.$/);
Copy link
Member

Choose a reason for hiding this comment

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

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 addaleax self-assigned this Feb 21, 2017
@DavidCai1111 DavidCai1111 force-pushed the test/vm-coverage branch 2 times, most recently from c96ab60 to 1d6d58d Compare February 22, 2017 02:57
@DavidCai1111
Copy link
Member Author

DavidCai1111 commented Feb 22, 2017

@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
Copy link
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
Copy link
Member Author

@addaleax OK, updated 👍

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@addaleax
Copy link
Member

Landed in c7205ba, thanks for the PR!

@addaleax addaleax closed this Feb 24, 2017
addaleax pushed a commit that referenced this pull request Feb 24, 2017
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
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 italoacasas mentioned this pull request Feb 25, 2017
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

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
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
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 MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants