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 fix #22487

Closed
wants to merge 3 commits into from
Closed

Test fix #22487

wants to merge 3 commits into from

Conversation

abelmark
Copy link
Contributor

@abelmark abelmark commented Aug 23, 2018

Fixes: #22472

Checklist

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 23, 2018
@lpinca
Copy link
Member

lpinca commented Aug 24, 2018

@abelmark can you please rebase against master and remove the merge commit? Thank you.

Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM after @lpinca comment is addressed.
Also, @abelmark you don't have you name/email set up correctly in git (github didn't recognize you as the author of the commit), so if you want to have 'contributor' github badge I'd reccomend you to do that (see https://help.github.com/articles/setting-your-username-in-git/)

@abelmark
Copy link
Contributor Author

I believe I set it up... the article you listed said that Git username is not the same as your Github identity... so I just put it as my full name. Will I still receive the contribution badge?

@@ -62,4 +62,5 @@ tests.forEach(function(test) {

assert.strictEqual(process.stdout.writeTimes, tests.length);

restoreStdout();
common.restoreStdout();
common.restoreStderr();
Copy link
Member

Choose a reason for hiding this comment

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

nit: newline at the end of the file

Copy link
Member

Choose a reason for hiding this comment

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

(I added it.)

Copy link
Member

Choose a reason for hiding this comment

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

@Trott this has to be: restoreStdout() and restoreStderr(). So the common. part has to be removed on both. restoreStderr has to be destructured along the other parts.

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR I didn't author any code. All I did here was add a missing newline at end of file. (I should have been more explicit about that in my previous comment.)

@lundibundi
Copy link
Member

@abelmark sorry, I thought the link included both name and email. I think your local git-config email doesn't match the one on github. Here is the link: https://help.github.com/articles/setting-your-commit-email-address-in-git/
Also, based on https://patch-diff.githubusercontent.com/raw/nodejs/node/pull/22487.patch you have From: Mark Abel <mark_abel@apple.com> as your local email, perhaps that's not the one you use in github. I believe if you just add this email to https://github.com/settings/emails (I think it doesn't need to be primary) GitHub will pick this commit as your and you will get the badge. (Another link: https://help.github.com/articles/adding-an-email-address-to-your-github-account/)

@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 27, 2018
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

@abelmark Appropriate functions (restoreStdout, restoreStderr) are in common/hijackstdio. See https://github.com/nodejs/node/blob/master/test/parallel/test-util-log.js#L28. They are not present in the usual require('../common').

@lundibundi lundibundi removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 29, 2018
Trott
Trott previously requested changes Sep 2, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

CI shows this breaking, for the reasons cited by @BridgeAR in #22487 (comment) and @lundibundi in #22487 (review). @abelmark Are you able to fix it the way they describe?

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

Ping @abelmark

abelmark and others added 3 commits September 19, 2018 11:27
bb29405 lib,src: fix consistent spacing inside braces
76340e3 test: fix RegExp nits
d00e5f1 test: add hijackStdout and hijackStderr
98e54b0 meta: restore original copyright header
3d2aef3 test: s/assert.equal/assert.strictEqual/
7a0e462 test: use eslint to fix var->const/let
ff1efa6 test: use const for all require() calls
9940766 test: fix tests after V8 upgrade
d1aabd6 test: fix style issues after eslint update
02fe821 test: load common.js to test for global leaks
e3f9335 tools: re-enable comma-spacing linter rule
f29762f test: enable linting for tests
3e1b1dd Remove excessive copyright/license boilerplate
0e19476 test: split test in parallel/sequential
Fixes: #22472
@lundibundi
Copy link
Member

Rebased and fixed the issue to get this going.
Upon landing we may add Co-authored-by if needed.
CI: https://ci.nodejs.org/job/node-test-pull-request/17314/

@addaleax
Copy link
Member

Landed in ed35df7, thanks for the PR! 🎉

@addaleax addaleax closed this Sep 19, 2018
addaleax pushed a commit that referenced this pull request Sep 19, 2018
Fixes: #22472
Co-authored-by: Denys Otrishko <shishugi@gmail.com>

PR-URL: #22487
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2018
Fixes: #22472
Co-authored-by: Denys Otrishko <shishugi@gmail.com>

PR-URL: #22487
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: help annotate test/parallel/test-util-log.js
9 participants