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

doc: fix example single-test command #29171

Conversation

davidguttman
Copy link
Contributor

@davidguttman davidguttman commented Aug 16, 2019

Changes example command to refer to the correct test directory: ./test/parallel/ instead of ./parallel/

Checklist

Changes example command to refer to the correct test directory: 
`./test/parallel/` instead of `./parallel/`
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Aug 16, 2019
@richardlau
Copy link
Member

Some history:

Originally the test runner specified tests to run by the name of the test suite (e.g. parallel) and optionally tests within it (e.g. parallel/test-fs-* to run all the tests in test/parallel beginning with test-fs-). Later on a change was made (#9694) to also allow specifying the path to the test file (e.g. test/parallel/test-stream2-transform.js). The result is that the runner is fairly lenient (e.g. you can omit the .js extensions if specifying a path) and the existing example will work.

I'll defer to others as to whether the example should be changed (both the original and the change made in this PR will be accepted by the test runner).

@nodejs-github-bot
Copy link
Collaborator

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.

Because there's no explanation that you can leave off the test/ (and, for that matter, the .js), it's probably best to just use the full path like this. I would prefer if the commit message is amended when we land this to say "improve" (or something similar) rather than "fix" as this isn't actually broken, IMO.

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

trivikr commented Aug 18, 2019

Should we also document that parallel/test-* works or should we deprecate it?

@Trott
Copy link
Member

Trott commented Aug 18, 2019

Should we also document that parallel/test-* works or should we deprecate it?

IMO, neither.

@Trott
Copy link
Member

Trott commented Aug 18, 2019

Should we also document that parallel/test-* works or should we deprecate it?

IMO, neither.

A little more detail: No need to deprecate. End users never use/see this tool. It's for us. No need to document that you can leave off test/ and .js. It's not significant. If people know about it, cool. If they don't, it's not hurting them. So, just leave it in (for people who are using it and like it) but don't document it because it unnecessarily complicates the documentation.

Trott pushed a commit that referenced this pull request Aug 18, 2019
Changes example command to refer to the full test directory:
`./test/parallel/` instead of `./parallel/`

PR-URL: #29171
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott
Copy link
Member

Trott commented Aug 18, 2019

Landed in 287c3ab.

Thanks for the contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants