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-repl-definecommand by Trott · Pull Request #17795 · nodejs/node

Conversation

@Trott

@Trott Trott commented Dec 20, 2017

Copy link
Copy Markdown
Member

The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

/cc @bmeck

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test repl

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Trott
test: refactor test-repl-definecommand
8ec342e 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.
@Trott Trott added the test Issues and PRs related to Node.js core tests and test infrastructure. label Dec 20, 2017
maclover7
maclover7 approved these changes Dec 20, 2017
View reviewed changes

@maclover7 maclover7 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.

SGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
BridgeAR
BridgeAR reviewed Dec 21, 2017
View reviewed changes
Comment thread test/parallel/test-repl-definecommand.js Outdated
assert(/\n\.say1 help for say1\n/.test(output),
'help for say1 not present');
assert.ok(/\n\.say1 help for say1\n/.test(output),
'help for say1 not present');

@BridgeAR BridgeAR Dec 21, 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.

This seems to be a independent change. I do not really see why this should be changed?

Sorry, something went wrong.

Uh oh!

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

@Trott Trott Dec 21, 2017

Copy link
Copy Markdown
Member 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.

I was trying to standardize on assert.ok() for tests, but I see I also missed one on another line. Anyway, yes, it's unnecessary, so I'll undo it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bmeck bmeck mentioned this pull request Dec 21, 2017
repl: support top-level await #15566
Closed
3 of 4 tasks
jasnell
jasnell approved these changes Dec 21, 2017
View reviewed changes
@Trott
Update test-repl-definecommand.js
309ab33
@Trott

Trott commented Dec 21, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott added a commit to Trott/io.js that referenced this pull request Dec 21, 2017
@Trott
test: refactor test-repl-definecommand
c90b10d 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: nodejs#17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott

Trott commented Dec 21, 2017

Copy link
Copy Markdown
Member Author

Landed in c90b10d
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott closed this Dec 21, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@Trott @MylesBorins
test: refactor test-repl-definecommand
add0a8b 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@Trott @MylesBorins
test: refactor test-repl-definecommand
c446c7b 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@Trott @MylesBorins
test: refactor test-repl-definecommand
dce7d7f 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
v9.4.0 proposal #18069
Merged
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@Trott @gibfahn
test: refactor test-repl-definecommand
81d063c 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@Trott @gibfahn
test: refactor test-repl-definecommand
6997a80 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn added land-on-v6.x labels Jan 24, 2018
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@Trott @gibfahn
test: refactor test-repl-definecommand
822e93e 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
v6.13.0 proposal #18342
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@Trott @MylesBorins
test: refactor test-repl-definecommand
425c1cd 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@Trott @MylesBorins
test: refactor test-repl-definecommand
be92172 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@Trott @MylesBorins
test: refactor test-repl-definecommand
f291bc1 
The test was writing to both REPL input and output but only checking
output. Sending output to both streams seems like it was an error.

PR-URL: #17795
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Trott deleted the refactor-definecommand branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments
@jasnell jasnell jasnell approved these changes
+1 more reviewer
@maclover7 maclover7 maclover7 approved these changes
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

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.

5 participants

@Trott @jasnell @maclover7 @BridgeAR @gibfahn
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.