FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Test repl reset event by AdriVanHoudt · Pull Request #16836 · nodejs/node · GitHub

/ node Public

Test repl reset event - #16836

Closed
AdriVanHoudt wants to merge 4 commits into
nodejs:masterfrom
AdriVanHoudt:test-repl-reset-event
ConversationCommits (4)ChecksFiles changed
Closed

Test repl reset event#16836
AdriVanHoudt wants to merge 4 commits into
nodejs:masterfrom
AdriVanHoudt:test-repl-reset-event

Conversation

AdriVanHoudt commented Nov 6, 2017

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2017

AdriVanHoudt commented Nov 6, 2017

Copy link
Copy Markdown
Contributor Author

I'm not super sure about the wording so any feedback welcome

mscdex added the repl Issues and PRs related to the REPL subsystem. label Nov 6, 2017

gireeshpunathil commented Nov 7, 2017

Copy link
Copy Markdown
Member

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

Trott reviewed Nov 13, 2017
View reviewed changes
Comment thread test/parallel/test-repl-reset-event.js Outdated
context.foo,
42,
'"foo" property is missing from REPL using global as context'
'"foo" property is different from REPL using global as context.' +

Trott Nov 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: Probably should be a space at the end of the string on this line or else a space at the beginning of the string on the next line? Same for two other instances above.

AdriVanHoudt Nov 14, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good point, tell me what'd prefer :P

addaleax Nov 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@AdriVanHoudt I think we prefer @Trott's suggested style for most of the codebase :)

Trott Nov 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@AdriVanHoudt Space at the end is more typical throughout the code base as far as I've noticed, so go with that I guess? But either would be fine.

AdriVanHoudt commented Dec 4, 2017

Copy link
Copy Markdown
Contributor Author

@Trott I added a space at the end of every line, let me know if I need to squash this

Trott commented Dec 4, 2017

Copy link
Copy Markdown
Member

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

AdriVanHoudt commented Dec 4, 2017

Copy link
Copy Markdown
Contributor Author

The space made line 47 to long 🤦‍♂️

Trott commented Dec 4, 2017

Copy link
Copy Markdown
Member

The space made line 47 to long 🤦‍♂️

😃 If you could wrap and fix, that would be great. (And if not, someone else like me can come along and do it when we're ready to land. But saving that person a few keystrokes and some time would be great.)

AdriVanHoudt commented Dec 5, 2017

Copy link
Copy Markdown
Contributor Author

Do you mean splitting the string in 3 parts instead of the current 2? I feel like eslint should be able to fix this 🙃

Trott commented Dec 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Do you mean splitting the string in 3 parts instead of the current 2?

Easiest fix is probably to remove the comma after context on line 47.

I feel like eslint should be able to fix this 🙃

ESLint docs indicate max-len is not a rule that can be auto-fixed.

AdriVanHoudt commented Dec 6, 2017

Copy link
Copy Markdown
Contributor Author

ESLint docs indicate max-len is not a rule that can be auto-fixed.

Ok that rule does more than I expected, shortsighted comment from me sorry

I updated the line, so let's hope it is good now

Trott commented Dec 6, 2017

Copy link
Copy Markdown
Member

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

apapirovski pushed a commit that referenced this pull request Dec 9, 2017
test: improve assert messages in repl-reset-event
bbcbcb8 
PR-URL: #16836
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

apapirovski commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

Landed in bbcbcb8

Thank you for the contribution @AdriVanHoudt!

apapirovski closed this Dec 9, 2017
AdriVanHoudt deleted the test-repl-reset-event branch December 11, 2017 07:14
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: improve assert messages in repl-reset-event
6a70640 
PR-URL: #16836
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: improve assert messages in repl-reset-event
39d8e44 
PR-URL: #16836
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
test: improve assert messages in repl-reset-event
d8eea81 
PR-URL: #16836
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
test: improve assert messages in repl-reset-event
a50366f 
PR-URL: #16836
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
test: improve assert messages in repl-reset-event
d5b5278 
PR-URL: #16836
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Merged
MylesBorins mentioned this pull request Dec 20, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
addaleax addaleax left review comments
jasnell jasnell approved these changes
gireeshpunathil gireeshpunathil approved these changes

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL