GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test: refactored context type err message to regex by Shinobi881 · Pull Request #12596 · nodejs/node

Conversation

Shinobi881 commented Apr 22, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          test: refactored context type err message to regex

24f9ae4

nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label

Apr 22, 2017

mscdex added the vm Issues and PRs related to the vm subsystem. label

Apr 22, 2017

mscdex reviewed

Apr 22, 2017

View reviewed changes

Comment thread

test/parallel/test-vm-context.js Outdated

		/^TypeError: contextifiedSandbox argument must be an object\.$/;

		[undefined, null, 0, 0.0, ''].forEach(function(e) {

mscdex Apr 22, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unnecessary whitespace here and in the same place in the second section.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vsemozhetbyt commented Apr 22, 2017

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig approved these changes

Apr 22, 2017

View reviewed changes

cjihrig left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM with a couple comments.

This should probably be generalized more to a single loop with an sandbox input value and expected error. That doesn't need to be done here though... unless you want to.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/parallel/test-vm-context.js Outdated

		/^TypeError: contextifiedSandbox argument must be an object\.$/;

		[undefined, null, 0, 0.0, ''].forEach(function(e) {

cjihrig Apr 22, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you remove this blank line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/parallel/test-vm-context.js Outdated

		/^TypeError: sandbox argument must have been converted to a context\.$/;

		[{}, []].forEach(function(e) {

cjihrig Apr 22, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same comment here about the blank line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Shinobi881 Apr 22, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@cjihrig that was my original thought, but i figured I'd skip the type checking and be more direct. I still have one more PR for this file and could change it then. What do you recommend?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig Apr 22, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Instead of two separate loops, you could do the following with nested arrays:

[[undefined, nonContextualSandboxErrorMsg], etc.].forEach()

Then, you pass the first element to runInContext, and use the second element as the expected error.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Shinobi881 Apr 22, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will do. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bengl added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label

Apr 22, 2017

Muhsin Abdul-Musawwir added 2 commits

April 22, 2017 13:11


          test: changed throw for runInNewContext to regex

1c7c87f


          test: changed type error throws to regex

c33fac3

vsemozhetbyt commented Apr 22, 2017

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell approved these changes

Apr 24, 2017

View reviewed changes

jasnell pushed a commit that referenced this pull request

Apr 24, 2017


          test: refactored context type err message to regex

ef6a7cf

PR-URL: #12596
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

jasnell commented Apr 24, 2017

Copy link

Copy Markdown

Member

Landed in ef6a7cf

Shinobi881 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell closed this

Apr 24, 2017

Trott commented Apr 25, 2017

Copy link

Copy Markdown

Member

Thanks for the contribution! 🎉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

evanlucas pushed a commit that referenced this pull request

Apr 25, 2017


          test: refactored context type err message to regex

528a811

PR-URL: #12596
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

evanlucas mentioned this pull request

May 1, 2017

v7.10.0 proposal #12775

Merged

evanlucas pushed a commit that referenced this pull request

May 1, 2017


          test: refactored context type err message to regex

86457ed

PR-URL: #12596
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

evanlucas pushed a commit that referenced this pull request

May 2, 2017


          test: refactored context type err message to regex

2021ea1

PR-URL: #12596
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

gibfahn added the land-on-v6.x label

May 16, 2017

gibfahn pushed a commit that referenced this pull request

May 16, 2017


          test: refactored context type err message to regex

bfb209b

PR-URL: #12596
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

MylesBorins pushed a commit that referenced this pull request

May 18, 2017


          test: refactored context type err message to regex

8044b83

PR-URL: #12596
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request

May 23, 2017

v6.11.0 proposal #13059

Merged

andrew749 pushed a commit to michielbaird/node that referenced this pull request

Jul 19, 2017


          test: refactored context type err message to regex

c435b10

PR-URL: nodejs/node#12596
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Reviewers

cjihrig

cjihrig approved these changes

jasnell

jasnell approved these changes

+1 more reviewer

mscdex

mscdex left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

code-and-learn

Issues related to the Code-and-Learn events and PRs submitted during the events.

test

Issues and PRs related to Node.js core tests and test infrastructure.

Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Shinobi881 commented Apr 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdex Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Apr 22, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

Shinobi881 Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

Shinobi881 Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Apr 22, 2017

Uh oh!

jasnell commented Apr 24, 2017

Uh oh!

Trott commented Apr 25, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Shinobi881 commented Apr 22, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.