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

doc: Change test option at STEP 5: Test in CONTRIBUTING.md #12830

Closed
wants to merge 7 commits into
base: master
from

Conversation

@kysnm
Contributor

kysnm commented May 4, 2017

Refs #12771

Checklist
Affected core subsystem(s)

none

@lpinca lpinca requested review from MylesBorins and removed request for MylesBorins May 4, 2017

Show outdated Hide outdated CONTRIBUTING.md
You can run tests in parallel with option `-J`
```text
$ python tools/test.py -J --mode=release parallel

This comment has been minimized.

@lpinca

lpinca May 4, 2017

Member

There is make test-parallel for this so I wouldn't add it.

@lpinca

lpinca May 4, 2017

Member

There is make test-parallel for this so I wouldn't add it.

This comment has been minimized.

@kysnm

kysnm May 4, 2017

Contributor

Thank you for reviewing.
I will remove this.

@kysnm

kysnm May 4, 2017

Contributor

Thank you for reviewing.
I will remove this.

@vsemozhetbyt

LGTM with a nit: line 191 is 81 characters long, and this may break approaching doc linting. Could you please wrap this line, say, before the last word?

@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 4, 2017

Contributor

I fix it.
and check to the make lint

Do I have to squash this branch?

Contributor

kysnm commented May 4, 2017

I fix it.
and check to the make lint

Do I have to squash this branch?

Show outdated Hide outdated CONTRIBUTING.md
$ python tools/test.py --mode=release parallel/test-stream2-transform
```
If you want to check the other option, please refer the help with option

This comment has been minimized.

@lpinca

lpinca May 4, 2017

Member

Nit: take this with a grain of salt as I'm not a native speaker but maybe it better to use "the other options, please refer to the help with the option".

@lpinca

lpinca May 4, 2017

Member

Nit: take this with a grain of salt as I'm not a native speaker but maybe it better to use "the other options, please refer to the help with the option".

This comment has been minimized.

@kysnm

kysnm May 4, 2017

Contributor

I agreed

@kysnm

kysnm May 4, 2017

Contributor

I agreed

@lpinca

lpinca approved these changes May 4, 2017

@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt May 4, 2017

Member

Thank you! I may squash if it is not difficult. Or this may be squashed before landing by somebody else.

Member

vsemozhetbyt commented May 4, 2017

Thank you! I may squash if it is not difficult. Or this may be squashed before landing by somebody else.

@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 4, 2017

Contributor

@vsemozhetbyt
I understand it 😄

Contributor

kysnm commented May 4, 2017

@vsemozhetbyt
I understand it 😄

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca May 4, 2017

Member

@kysnm I think the last commit removed more words than it had to :)

Member

lpinca commented May 4, 2017

@kysnm I think the last commit removed more words than it had to :)

@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 4, 2017

Contributor

@lpinca
I'm sorry 💦
Does this commit is match with your pointed out?

Contributor

kysnm commented May 4, 2017

@lpinca
I'm sorry 💦
Does this commit is match with your pointed out?

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca May 4, 2017

Member

Yes, thank you!

Member

lpinca commented May 4, 2017

Yes, thank you!

@refack

refack approved these changes May 4, 2017

@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 4, 2017

Contributor

Thank you for your reviewing.
I'm glad to can contribute this 🎉

Contributor

kysnm commented May 4, 2017

Thank you for your reviewing.
I'm glad to can contribute this 🎉

Show outdated Hide outdated CONTRIBUTING.md
$ python tools/test.py --mode=release parallel/test-stream2-transform
```
If you want to check the other option, please refer to the help with the option

This comment has been minimized.

@aqrln

aqrln May 4, 2017

Member

"...other options..."

@aqrln

aqrln May 4, 2017

Member

"...other options..."

This comment has been minimized.

@aqrln

aqrln May 4, 2017

Member

Also, can you please avoid using personal pronouns like "you"?

EDIT: on the other hand, that's how this document is already written, and such wording is common in it.

@aqrln

aqrln May 4, 2017

Member

Also, can you please avoid using personal pronouns like "you"?

EDIT: on the other hand, that's how this document is already written, and such wording is common in it.

This comment has been minimized.

@kysnm

kysnm May 4, 2017

Contributor

I see

@kysnm

kysnm May 4, 2017

Contributor

I see

Show outdated Hide outdated CONTRIBUTING.md
```
If you want to check the other option, please refer to the help with the option
`--help`

This comment has been minimized.

@aqrln

aqrln May 4, 2017

Member

Unnecessary space at the beginning of the line.

@aqrln

aqrln May 4, 2017

Member

Unnecessary space at the beginning of the line.

This comment has been minimized.

@kysnm

kysnm May 4, 2017

Contributor

I see

@kysnm

kysnm May 4, 2017

Contributor

I see

@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 4, 2017

Contributor

Fixed

Contributor

kysnm commented May 4, 2017

Fixed

@watilde

watilde approved these changes May 5, 2017

@jasnell

jasnell approved these changes May 5, 2017

Show outdated Hide outdated CONTRIBUTING.md
@@ -185,7 +185,14 @@ If you are updating tests and just want to run a single test to check it, you
can use this syntax to run it exactly as the test harness would:
```text
$ python tools/test.py -v --mode=release parallel/test-stream2-transform
$ python tools/test.py --mode=release parallel/test-stream2-transform

This comment has been minimized.

@gibfahn

gibfahn May 5, 2017

Member

Can we add a -J? Given that it says "exactly as the test harness would" we should probably be exact:

-$ python tools/test.py --mode=release parallel/test-stream2-transform
+$ python tools/test.py -J --mode=release parallel/test-stream2-transform

LGTM otherwise.

@gibfahn

gibfahn May 5, 2017

Member

Can we add a -J? Given that it says "exactly as the test harness would" we should probably be exact:

-$ python tools/test.py --mode=release parallel/test-stream2-transform
+$ python tools/test.py -J --mode=release parallel/test-stream2-transform

LGTM otherwise.

This comment has been minimized.

@aqrln

aqrln May 6, 2017

Member

I think it would be run "exactly as the test harness would" either way given that tools/test.py is the test harness, regardless of the options.

@aqrln

aqrln May 6, 2017

Member

I think it would be run "exactly as the test harness would" either way given that tools/test.py is the test harness, regardless of the options.

This comment has been minimized.

@kysnm

kysnm May 6, 2017

Contributor

I think that users would understand by looking at help.

@kysnm

kysnm May 6, 2017

Contributor

I think that users would understand by looking at help.

This comment has been minimized.

@gibfahn

gibfahn May 6, 2017

Member

I think it would be run "exactly as the test harness would" either way given that tools/test.py is the test harness, regardless of the options.

I assume the test harness means make test, otherwise that part of the sentence is kinda meaningless. The reason I think this is necessary is that using -J can cause subtle bugs (when tests interact with each other), and that is impossible to debug if you don't use the flag by default.

@gibfahn

gibfahn May 6, 2017

Member

I think it would be run "exactly as the test harness would" either way given that tools/test.py is the test harness, regardless of the options.

I assume the test harness means make test, otherwise that part of the sentence is kinda meaningless. The reason I think this is necessary is that using -J can cause subtle bugs (when tests interact with each other), and that is impossible to debug if you don't use the flag by default.

This comment has been minimized.

@aqrln

aqrln May 6, 2017

Member

The reason I think this is necessary is that using -J can cause subtle bugs (when tests interact with each other)

Yeah, I agree with that if we are talking about running multiple tests. But that sentence is about running a single test, isn't it? Do I miss the point?

@aqrln

aqrln May 6, 2017

Member

The reason I think this is necessary is that using -J can cause subtle bugs (when tests interact with each other)

Yeah, I agree with that if we are talking about running multiple tests. But that sentence is about running a single test, isn't it? Do I miss the point?

This comment has been minimized.

@gibfahn

gibfahn May 6, 2017

Member

Wouldn't a PR changing -J to default (with -j1 as the opt-out) make sense?

Maaaybe, I think I'd be +1 on that, but I suspect it might be contentious...

@gibfahn

gibfahn May 6, 2017

Member

Wouldn't a PR changing -J to default (with -j1 as the opt-out) make sense?

Maaaybe, I think I'd be +1 on that, but I suspect it might be contentious...

This comment has been minimized.

@refack

refack May 6, 2017

Member

Other option, a wrapper script test-ci (.sh & .cmd).
This one I'm doing

@refack

refack May 6, 2017

Member

Other option, a wrapper script test-ci (.sh & .cmd).
This one I'm doing

This comment has been minimized.

@gibfahn

gibfahn May 6, 2017

Member

Other option, a wrapper script test-ci (.sh & .cmd).

Doesn't that duplicate the Makefile and vcbuild.bat test-ci target? What's the gain?

@gibfahn

gibfahn May 6, 2017

Member

Other option, a wrapper script test-ci (.sh & .cmd).

Doesn't that duplicate the Makefile and vcbuild.bat test-ci target? What's the gain?

This comment has been minimized.

@refack

refack May 6, 2017

Member

It'll take $*, so it's useful for single files, or single suits.
I don't know about make test-ci but vcbuild test-ci builds first (with Feet of clay), very demotivating.

@refack

refack May 6, 2017

Member

It'll take $*, so it's useful for single files, or single suits.
I don't know about make test-ci but vcbuild test-ci builds first (with Feet of clay), very demotivating.

This comment has been minimized.

@refack

refack May 6, 2017

Member

PTAL #12874

@refack
Show outdated Hide outdated CONTRIBUTING.md
If you want to check the other option, please refer to the help with the option
`--help`
If you want to check the other options, please refer to the help with the
option `--help`

This comment has been minimized.

@refack

refack May 6, 2017

Member

Nit: please refer to the help with the option `--help` => please refer to the documentation by using the `--help` option

@refack

refack May 6, 2017

Member

Nit: please refer to the help with the option `--help` => please refer to the documentation by using the `--help` option

This comment has been minimized.

@kysnm

kysnm May 7, 2017

Contributor

I think the word documentation gives the users impression that they should see the https://nodejs.org/en/docs/

@kysnm

kysnm May 7, 2017

Contributor

I think the word documentation gives the users impression that they should see the https://nodejs.org/en/docs/

This comment has been minimized.

@refack

refack May 7, 2017

Member

Several options

  1. please refer to the built in documentation by using the `--help` option
  2. please refer to the help by using the `--help` option
  3. please refer to the help by passing the `--help` flag
@refack

refack May 7, 2017

Member

Several options

  1. please refer to the built in documentation by using the `--help` option
  2. please refer to the help by using the `--help` option
  3. please refer to the help by passing the `--help` flag

This comment has been minimized.

@kysnm

kysnm May 7, 2017

Contributor

Thank you for suggestions
I will change to the No.2

@kysnm

kysnm May 7, 2017

Contributor

Thank you for suggestions
I will change to the No.2

@refack refack referenced this pull request May 6, 2017

Closed

test, build: add test-ci wrappers #12874

3 of 3 tasks complete
@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 10, 2017

Contributor

Is there anything i should fix the patch, yet?
Please let me know 🙏

Contributor

kysnm commented May 10, 2017

Is there anything i should fix the patch, yet?
Please let me know 🙏

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn May 10, 2017

Member

I'd still like the -J (unless someone is -1 on it).

-$ python tools/test.py --mode=release parallel/test-stream2-transform
+$ python tools/test.py -J --mode=release parallel/test-stream2-transform
Member

gibfahn commented May 10, 2017

I'd still like the -J (unless someone is -1 on it).

-$ python tools/test.py --mode=release parallel/test-stream2-transform
+$ python tools/test.py -J --mode=release parallel/test-stream2-transform
@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 11, 2017

Contributor

Added the option -J, because no one is -1 on comment above.

Contributor

kysnm commented May 11, 2017

Added the option -J, because no one is -1 on comment above.

@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 11, 2017

Contributor

@aqrln
How do you think?

Contributor

kysnm commented May 11, 2017

@aqrln
How do you think?

@aqrln

aqrln approved these changes May 11, 2017

LGTM, thanks!

@kysnm

This comment has been minimized.

Show comment
Hide comment
@kysnm

kysnm May 12, 2017

Contributor

Thanks for all reviewers.
I'm looking forward to that this patch will be landed
(if there is no more point out 😁 )

Contributor

kysnm commented May 12, 2017

Thanks for all reviewers.
I'm looking forward to that this patch will be landed
(if there is no more point out 😁 )

refack added a commit to refack/node that referenced this pull request May 12, 2017

doc: Change options at STEP 5 in CONTRIBUTING.md
PR-URL: nodejs#12830
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 12, 2017

Member

Landed in d7d16f7

Member

refack commented May 12, 2017

Landed in d7d16f7

@refack refack closed this May 12, 2017

@kysnm kysnm deleted the kysnm:change-test-option branch May 12, 2017

anchnk added a commit to anchnk/node that referenced this pull request May 19, 2017

doc: Change options at STEP 5 in CONTRIBUTING.md
PR-URL: nodejs#12830
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

MylesBorins added a commit that referenced this pull request Jun 22, 2017

doc: Change options at STEP 5 in CONTRIBUTING.md
PR-URL: #12830
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

MylesBorins added a commit that referenced this pull request Jul 11, 2017

doc: Change options at STEP 5 in CONTRIBUTING.md
PR-URL: #12830
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

@MylesBorins MylesBorins referenced this pull request Jul 18, 2017

Merged

v6.11.2 proposal #14356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment