Skip to content

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

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

Closed
kysnm 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
Prev Previous commit
Next Next commit
Change the word more naturally
  • Loading branch information
@kysnm
kysnm committed May 7, 2017
commit 2d522439ada3be733f1b89a6c99fb89d73ba70c8
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
Copy Markdown
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
Copy Markdown
Contributor

@aqrln aqrln May 6, 2017

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
Copy Markdown
Contributor Author

@kysnm kysnm May 6, 2017

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
Copy Markdown
Member

@gibfahn gibfahn May 6, 2017

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
Copy Markdown
Contributor

@aqrln aqrln May 6, 2017

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
Copy Markdown
Member

@gibfahn gibfahn May 6, 2017

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
Copy Markdown
Contributor

@refack refack May 6, 2017

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
Copy Markdown
Member

@gibfahn gibfahn May 6, 2017

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
Copy Markdown
Contributor

@refack refack May 6, 2017

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
Copy Markdown
Contributor

@refack refack May 6, 2017

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 options, please refer to the help with the
option `--help`
If you want to check the other options, please refer to the help by using
the `--help` option

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