GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: refactor test-cli-eval.js by cjihrig · Pull Request #10898 · nodejs/node

Conversation

@cjihrig

@cjihrig cjihrig commented Jan 19, 2017

Copy link
Copy Markdown
Contributor
  • Add missing common.mustCall().
  • Isolate tests using block statements where helpful.
  • Cleanup comments.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added dont-land-on-v7.x test Issues and PRs related to Node.js core tests and test infrastructure. labels Jan 19, 2017
@mscdex mscdex added process Issues and PRs related to the process subsystem. and removed dont-land-on-v7.x labels Jan 19, 2017
jasnell
jasnell approved these changes Jan 19, 2017
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

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 nit

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-cli-eval.js Outdated

@jasnell jasnell Jan 19, 2017

Copy link
Copy Markdown
Member

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.

Would be a bit more concise to use:

`${nodejs} --eval "require('${filename}')"`

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gibfahn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Trott
Trott approved these changes Jan 20, 2017
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

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 if CI is green

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@cjihrig
cjihrig force-pushed the test branch from c30b368 to 31e6688 Compare January 23, 2017 16:17
@nodejs-github-bot nodejs-github-bot added the dont-land-on-v7.x label Jan 23, 2017
@cjihrig

cjihrig commented Jan 23, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

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

EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/6003/
EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/6005/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas italoacasas removed the dont-land-on-v7.x label Jan 23, 2017
@cjihrig
test: refactor test-cli-eval.js
87039e3 
PR-URL: nodejs#10898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@cjihrig
cjihrig force-pushed the test branch from 31e6688 to 87039e3 Compare January 23, 2017 18:21
@nodejs-github-bot nodejs-github-bot added the dont-land-on-v7.x label Jan 23, 2017
@cjihrig
cjihrig merged commit 87039e3 into nodejs:master Jan 23, 2017
@cjihrig
cjihrig deleted the test branch January 23, 2017 18:22
@targos targos removed the dont-land-on-v7.x label Jan 28, 2017
targos pushed a commit that referenced this pull request Jan 28, 2017
@cjihrig @targos
test: refactor test-cli-eval.js
b8d3cde 
PR-URL: #10898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
v7.5.0 proposal #11062
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@cjihrig Italo A. Casas
test: refactor test-cli-eval.js
efa9845 
PR-URL: nodejs#10898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@cjihrig Italo A. Casas
test: refactor test-cli-eval.js
d157b57 
PR-URL: nodejs#10898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@jasnell

jasnell commented Mar 8, 2017

Copy link
Copy Markdown
Member

This will need backport PRs if it should land on v6 or v4
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added dont-land-on-v4.x labels Mar 8, 2017
@MylesBorins MylesBorins added land-on-v6.x and removed dont-land-on-v6.x labels Apr 18, 2017
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@cjihrig @MylesBorins
test: refactor test-cli-eval.js
627107a 
PR-URL: #10898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@cjihrig @MylesBorins
test: refactor test-cli-eval.js
2ee2cc6 
PR-URL: #10898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
v6.10.3 proposal #12498
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@cjihrig @andrew749
test: refactor test-cli-eval.js
5731206 
PR-URL: nodejs/node#10898
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

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

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.

8 participants

@cjihrig @jasnell @Trott @mscdex @MylesBorins @targos @italoacasas @nodejs-github-bot
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.