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-vm-is-context.js : changed the assert.thows object to regex and … #12785

Closed
wants to merge 5 commits into from

Conversation

@Jeyanthinath
Copy link
Contributor

commented May 2, 2017

…added score for sanbox object

First Pull request ...

In assert.throws changed the TypeError constructor to Regex .
Aslo added Scope for Sandbox Object

@mscdex mscdex added the vm label May 2, 2017

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

@@ -26,14 +26,16 @@ const vm = require('vm');

assert.throws(function() {
vm.isContext('string is not supported');
}, TypeError);
}, /TypeError/);

This comment has been minimized.

Copy link
@lpinca

lpinca May 2, 2017

Member

Can you please change this to: /^TypeError: sandbox must be an object$/?

This comment has been minimized.

Copy link
@Jeyanthinath

Jeyanthinath May 2, 2017

Author Contributor

I did changed ! thanks

const sandbox = { foo: 'bar' };
vm.createContext(sandbox);
assert.strictEqual(vm.isContext(sandbox), true);
{

This comment has been minimized.

Copy link
@lpinca

lpinca May 2, 2017

Member

I see no reasons to add this block.

This comment has been minimized.

Copy link
@Jeyanthinath

Jeyanthinath May 2, 2017

Author Contributor

I thought it would restrict the access of sandbox variable. @lpinca any suggestion ?

This comment has been minimized.

Copy link
@lpinca

lpinca May 2, 2017

Member

It might be useful if you want to reuse the sandbox variable in another scope but currently there is no need for it.

This comment has been minimized.

Copy link
@mscdex

mscdex May 2, 2017

Contributor

I don't think it really matters either way, but I personally would probably opt to just wait until someone adds another test below it that also uses variables.

This comment has been minimized.

Copy link
@Jeyanthinath

Jeyanthinath May 2, 2017

Author Contributor

ok , I will revoke it as of now

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

The commit messages should adhere to the commit guidelines.

vm: changed the error from constructor to regular expression
assert.throws initally throws the TypeError constructor and now replacing that with regular expression
const sandbox = { foo: 'bar' };
vm.createContext(sandbox);
assert.strictEqual(vm.isContext(sandbox), true);
const sandbox = { foo: 'bar' };

This comment has been minimized.

Copy link
@lpinca

lpinca May 2, 2017

Member

@Jeyanthinath please revert also these white space changes.

This comment has been minimized.

Copy link
@Jeyanthinath

Jeyanthinath May 2, 2017

Author Contributor

oops , I missed those , thanks !

vm: changed the error from constructor to regular expression
assert.throws initally throws the TypeError constructor and now replacing that with regular expression
@lpinca

This comment has been minimized.

Copy link
Member

commented May 2, 2017

How about using something like this for commit message?

test: add regex check in test-vm-is-context

Use a regex to validate the error message.
@lpinca
lpinca approved these changes May 2, 2017
@cjihrig
cjihrig approved these changes May 2, 2017
@addaleax
Copy link
Member

left a comment

LGTM, thank you for fixing this!

@jasnell
jasnell approved these changes May 2, 2017
@lpinca

This comment has been minimized.

Copy link
Member

commented May 5, 2017

lpinca added a commit to lpinca/node that referenced this pull request May 5, 2017
test: add regex check in test-vm-is-context
Use a regex to validate the error message.

PR-URL: nodejs#12785
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca

This comment has been minimized.

Copy link
Member

commented May 5, 2017

Landed in bc05436.

@lpinca lpinca closed this May 5, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
test: add regex check in test-vm-is-context
Use a regex to validate the error message.

PR-URL: nodejs#12785
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
gibfahn added a commit that referenced this pull request Jun 18, 2017
test: add regex check in test-vm-is-context
Use a regex to validate the error message.

PR-URL: #12785
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn added a commit that referenced this pull request Jun 20, 2017
test: add regex check in test-vm-is-context
Use a regex to validate the error message.

PR-URL: #12785
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: add regex check in test-vm-is-context
Use a regex to validate the error message.

PR-URL: #12785
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.