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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ce884c0
doc: Change test option at STEP 5: Test in CONTRIBUTING.md
kysnm May 4, 2017
7832bb6
Remove unnecessary sentence
kysnm May 4, 2017
def6cf0
Follow the doc linting
kysnm May 4, 2017
c05c325
Fix to words more better
kysnm May 4, 2017
fd8324e
Fix to plural
kysnm May 4, 2017
2d52243
Change the word more naturally
kysnm May 7, 2017
f7a15d7
Add option `-J`
kysnm May 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,8 @@ can use this syntax to run it exactly as the test harness would:
$ python tools/test.py --mode=release parallel/test-stream2-transform
Copy link
Member

@gibfahn gibfahn May 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that users would understand by looking at help.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL #12874

```

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`
Copy link
Contributor

@refack refack May 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


```text
$ python tools/test.py --help
Expand Down