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: use strictEqual() domain-http #9996

Closed

Conversation

cdnadmin
Copy link
Contributor

@cdnadmin cdnadmin commented Dec 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

did this at nina 2016 na code and learn event

did this at nina 2016 na code and learn event
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@mscdex mscdex added the domain Issues and PRs related to the domain subsystem. label Dec 1, 2016
@gibfahn
Copy link
Member

gibfahn commented Dec 1, 2016

Your name on this commit currently shows cdnadmin. People normally use their full name, although Github username is fine if you don't want to.

To change, you can do:

git commit --amend --no-edit --author="Monico Moreno <[email protected]>"

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Commit message could be improved, but LGTM

@cdnadmin
Copy link
Contributor Author

cdnadmin commented Dec 2, 2016

@gibfahn thx for the suggestion; I think will do what you mentioned from here on. For this go-round, since you're good with just GH username, I'm going to leave it as-is.

@cjihrig I agree that the commit message could be improved. We were under a bit of time crunch due to lack of wif-fi bandwidth at Code & Learn. I'll leave it as-is since you had mercy and gave it a LGTM ;-).

Thx y'all!

@Trott
Copy link
Member

Trott commented Dec 2, 2016

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

@gibfahn
Copy link
Member

gibfahn commented Dec 2, 2016

@cdnadmin I suggest you run this on your machine (it'll change your defaults so you never have to worry about it again).

git config --global user.name "Monico Moreno"
git config --global user.email "[email protected]"

@cdnadmin
Copy link
Contributor Author

cdnadmin commented Dec 3, 2016

Cool! Thx, @gibfahn ! 👍 I actually already have my git defaults set, but I typically set the user.name and user.email props locally and leave it at that. Now that I'm going to try and contribute actively to node, I might set my defaults globally, like you suggested above.

@cdnadmin
Copy link
Contributor Author

cdnadmin commented Dec 3, 2016

Hey, @Trott RE:Jenkins

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

Not sure if there is anything I need to do from my end or just wait for this PR to be landed.

Standing by for advice ;-) ...

@gibfahn
Copy link
Member

gibfahn commented Dec 3, 2016

@cdnadmin It's all green, so you should be good. We normally let PRs sit for at least 48 hours, so (hopefully) someone will merge this in in a day or two if no-one objects.

@cdnadmin
Copy link
Contributor Author

cdnadmin commented Dec 3, 2016

Thx, @gibfahn, yup, I'm green in more ways than one! Ha! Kidding!

Looking forward to seeing this bad boy landed! 💪🏻

jasnell pushed a commit that referenced this pull request Dec 5, 2016
did this at nina 2016 na code and learn event

PR-URL: #9996
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

Landed in 40daf6b. Thank you for the PR and for participating in the code-and-learn!

@jasnell jasnell closed this Dec 5, 2016
Fishrock123 pushed a commit that referenced this pull request Dec 6, 2016
did this at nina 2016 na code and learn event

PR-URL: #9996
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
did this at nina 2016 na code and learn event

PR-URL: nodejs#9996
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
did this at nina 2016 na code and learn event

PR-URL: nodejs#9996
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
did this at nina 2016 na code and learn event

PR-URL: #9996
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
did this at nina 2016 na code and learn event

PR-URL: #9996
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
did this at nina 2016 na code and learn event

PR-URL: #9996
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants