-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: make it easier to run tests for subsystems #15450
Conversation
@@ -420,6 +427,29 @@ $ ./node ./test/parallel/test-stream2-transform.js | |||
Remember to recompile with `make -j4` in between test runs if you change code in | |||
the `lib` or `src` directories. | |||
|
|||
##### Test Coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Trott I revamped the notes about coverage a little bit, and to me the note feels appropriate in CONTRIBUTING.md now -- my thinking being it directs a contributor toward healthy practices.
CONTRIBUTING.md
Outdated
$ make coverage | ||
``` | ||
|
||
A detailed coverage report will be output to `coverage/index.html`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, if you want C++ coverage too, I think you need ./configure --coverage
as well, and then (?) you’ll get coverage/cxxcoverage.html
as well – that might not be necessary but maybe we should mention it?
tools/test.py
Outdated
if suite == 'default': | ||
paths += default_suites | ||
else: | ||
path = SplitPath(NormalizePath("*/test-%s-*" % (suite))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest just using "*/*%s*"
, so that we get e.g. test/parallel/test-cluster-dgram-1.js
even if we specify just dgram
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this approach feels a bit like it might be too much magic :) Not sure what to do about that, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that it definitely feels like the right move to add suites_default
, as it allows folks to stop manually updating a variable in several places whenever a new test folder is added ... and, if we already have this shorthand added, it feels really elegant to be able to also specific a specific suite to run like http
.
An alternative might be something like --suites=default
, --suites=http
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is the good kind of magic, all you have to remember is it's basically doing a search for the string you provide right?
As long as it still works with the existing uses:
tools/test.py parallel
tools/test.py parallel/test-assert
tools/test.py test/parallel/test-assert.js
which it will, I think this will be a great addition. I'm already looking forward to being able to do tools/test.py foo
.
EDIT: I didn't actually read through the code carefully enough. Is there a reason to require tools/test.py suites_foo
rather than just tools/test.py foo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibfahn my concern was if there's a collision on a folder name, e.g., if I want to run addons
. Perhaps check if the arg
is in the list of folders, and then assume it's the name of a suite otherwise? I will happily implement whichever magic folks land on ... it would definitely be nice to not have the suites_
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you said sounds reasonable, but probably worth waiting to see what others think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibfahn @addaleax @refack okay, riffing on @gibfahn's idea what if instead of requiring the subsystem_
prefix, we did something clever like this:
def MaybeSubsystem(test_root, arg):
if (arg == 'default'):
return True
test_dirs = os.listdir(test_root)
subsystem_regex = re.compile(r'^[a-zA-Z-]*$')
return subsystem_regex.match(arg) and (arg not in test_dirs)
If the arg
passed to tool/test.py
matches a test-folder, or contains special characters like /
or *
we execute tests like usual. If the arg
doesn't match a folder, and doesn't contain special characters, assume you're trying to test a subsystem ... at this point you'd be able to:
python tools/test.py http child-process
or, you can always use the old approach of:
python tools/test.py parallel/test-assert
and things would be have as usual ...
I think this small change would make this a pretty darn slick feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnoordhuis @refack @gibfahn pushed the slight refactor that eliminates the need for a subsystem_
prefix, it definitely fees much more elegant to me now. It's pretty intuitive that I could write:
python tools/test.py http2
and get tests for http2
.
I had an idea floating in my head to split the test files (at least in parallel) into "sub-suites", i.e. $tools/test.py parallel
$tools/test.py parallel/assert/
$tools/test.py test/parallel/test-assert.js
:: and the new magic
$tools/test.py *foo |
/cc @nodejs/testing |
CONTRIBUTING.md
Outdated
@@ -420,6 +427,30 @@ $ ./node ./test/parallel/test-stream2-transform.js | |||
Remember to recompile with `make -j4` in between test runs if you change code in | |||
the `lib` or `src` directories. | |||
|
|||
##### Test Coverage | |||
|
|||
It's good practice to try to improve test coverage, as you add new features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no comma after coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd be inclined to frame this more as making sure you're new code is 100% covered by tests. The way it's written now, it might be interpreted as "While you're in there making a change, why don't you add some tests for other uncovered bits of code?" (And I'd dispute that doing so is "good practice".)
Maybe something like this?: "It's a good idea to confirm that any added or changed code is completely covered by tests."
CONTRIBUTING.md
Outdated
`CI_NATIVE_SUITES` variables: | ||
|
||
```text | ||
$ CI_JS_SUITES=suites_child-process CI_NATIVE_SUITES= make coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ make coverage CI_JS_SUITES=suites_child-process CI_NATIVE_SUITES=
- also works with shells that don't support VAR=value cmd
syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know they existed, which shells are those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least csh
and tcsh
don't. (Not that I expect many people use those but still.)
tools/test.py
Outdated
@@ -1565,6 +1549,25 @@ def PrintCrashed(code): | |||
else: | |||
return "CRASHED (Signal: %d)" % -code | |||
|
|||
# these suites represent special cases that should not be run as part of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"These" (capital)
tools/test.py
Outdated
def DefaultSuites(test_root): | ||
built_in_tests = [] | ||
for testdir in os.listdir(test_root): | ||
if isdir(join(test_root, testdir)) and (testdir not in IGNORED_SUITES): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paren are not necessary around the not in
expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paren are not necessary around the not in expression.
Also a quick look at the file shows a tendency not to use them in multi termed contininals:
https://github.com/nodejs/node/blob/bf6c7eaeacf8c1ceee5c80861d10cecfe656e36b/tools/test.py#L652
I miss python
tools/test.py
Outdated
repositories = [TestRepository(join(workspace, 'test', name)) for name in suites] | ||
repositories += [TestRepository(a) for a in options.suite] | ||
|
||
root = LiteralTestSuite(repositories) | ||
default_suites = DefaultSuites(test_root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: I refactored this a little bit and got:
paths = ArgsToTestPaths(test_root, args, suites)
...
#Instead of DefaultSuites and MaybeSubsystem
def ArgsToTestPaths(test_root, args, suites):
if len(args) == 0 or 'default' in args:
def_suites = filter(lambda s: s not in IGNORED_SUITES, suites)
args = filter(lambda a: a != 'default', args) + def_suites
subsystem_regex = re.compile(r'^[a-zA-Z-]*$')
check = lambda arg: subsystem_regex.match(arg) and (arg not in suites)
mapped_args = ["*/test*-%s-*" % arg if check(arg) else arg for arg in args]
paths = [SplitPath(NormalizePath(a)) for a in mapped_args]
return paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you thinking we'd pull the IGNORED_SUITES logic into GetSuites()?'
EDIT: the ignore logic replaces the whitelisting of test-suites that was being performed in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I swallowed that...
I think restoring it here is also good.
[updated snippet above]
29a057a
to
c6901c0
Compare
``` | ||
|
||
The above command executes tests for the `child-process` subsystem and | ||
outputs the resulting coverage report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to keep nits coming on this, but one more: People new to using make coverage
will probably want to know that when you're done, you should run make coverage-clean
(and probably ./configure
for good measure) to get things back to the non-modified-for-coverage state. Maybe something like this?:
Running coverage tests will create many directories and files. To clean up these new directories and files, run
make coverage-clean
. To avoid accidentally using the executable that has been modified for coverage reports, you may wish to also run./configure && make -j4
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, appreciate the thorough review. I just pushed your recommendations, let me know what you think and I'll rebse.
@@ -420,6 +427,28 @@ $ ./node ./test/parallel/test-stream2-transform.js | |||
Remember to recompile with `make -j4` in between test runs if you change code in | |||
the `lib` or `src` directories. | |||
|
|||
##### Test Coverage | |||
|
|||
It's good practice to ensure any code you add or change is covered by tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add sentence here along these lines:
Note that generating a test coverage report can take several minutes.
|
||
```text | ||
$ ./configure --coverage && make coverage | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe add something like this?
You may see tests failing that normally succeed when not generating a coverage report. Do not be alarmed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the whole suite runs with coverage enabled. So I'd be tempted to leave out this caveat unless folks to start noticing this behavior ... I'm also happy on the nyc/Istanbul side of things (which I have the commit bit for) to fix any issues that do consistently crop up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @bcoe , I'd rather we treated coverage test failures as bugs if possible.
CONTRIBUTING.md
Outdated
$ ./configure --coverage && make coverage | ||
``` | ||
|
||
A detailed coverage report will be output to `coverage/index.html` for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro-nit: will be output to
-> will be written to
Left more nits but I don't consider any of them blocking. |
Seems like there are more errors in the es-module. https://ci.nodejs.org/job/node-test-commit-smartos/11651/nodes=smartos16-64/console
|
036a027
to
04b90f1
Compare
You can now run suites for subsystem using shorthand, e.g., http. Switch to black-list of default test folders from white-list. Tests run by 'make test', 'make coverage', etc., now configurable. Stop running known_issues suite when collecting test coverage.
04b90f1
to
666f20e
Compare
hmm still quite a bit of red in this CI run. Can you take a look to make sure none of the failures are relevant? |
Running CI again to see if the same types of failures recur. (They're not on master. I just ran a |
@Trott @jasnell I added
fun future work, try to get some of these flaky suites working on all platforms. |
You can now run suites for subsystem using shorthand, e.g., http. Switch to black-list of default test folders from white-list. Tests run by 'make test', 'make coverage', etc., now configurable. Stop running known_issues suite when collecting test coverage. PR-URL: nodejs#15450 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 5be4dfa |
You can now run suites for subsystem using shorthand, e.g., http. Switch to black-list of default test folders from white-list. Tests run by 'make test', 'make coverage', etc., now configurable. Stop running known_issues suite when collecting test coverage. PR-URL: #15450 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
You can now run suites for subsystem using shorthand, e.g., http. Switch to black-list of default test folders from white-list. Tests run by 'make test', 'make coverage', etc., now configurable. Stop running known_issues suite when collecting test coverage. PR-URL: #15450 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
You can now run suites for subsystem using shorthand, e.g., http. Switch to black-list of default test folders from white-list. Tests run by 'make test', 'make coverage', etc., now configurable. Stop running known_issues suite when collecting test coverage. PR-URL: nodejs/node#15450 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
This seems to have caused an issue for the coverage job in that I don't think the addon tests run anymore. I've not had a chance to check yet but I'm concerned that we may not be running them in the regular CI runs unless all of the jobs except for the coverage one was updated. @BridgeAR do you know if there were any updates to the CI jobs to go along with landing this ? |
#16132 restores some suites back for the |
Pulling from 8.x until the fix lands |
You can now run suites for subsystem using shorthand, e.g., http. Switch to black-list of default test folders from white-list. Tests run by 'make test', 'make coverage', etc., now configurable. Stop running known_issues suite when collecting test coverage. PR-URL: #15450 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
This does not land cleanly on v6.x does it make sense to backport? if so please include #16132 |
Problem statement
subsystem, e.g.,
http
, when their tests are spread out acrossmultiple folders (
pummel/
,sequential/
, etc.).tests/subsystems, speeding up the test coverage feedback loop.
Potential solution: split tests into subsystem-specific folders
this was proposed in #15437.
It became apparent in #15437, and in the discussion surrounding it, that
splitting tests into subsystem-specific folders potentially creates more
problems than it presents benefits:
to what folder a test should be placed in (what if a test interacts
with two subsystems?).
Proposed solution: introduce suites_<subsystem> shorthand, make
CI_JS_SUITES
CI_NATIVE_SUITES
configurableRather than reorganizing tests into an alternative folder structure, this
pull request introduces the concept of a
suites_<subsystem>
shorthand totest.py
.Running:
Would result in any tests matching the
*/test-child-process-*
pattern beingexecuted, regardless of whether they are contained in
test/sequential
,test/parallel
, etc.Other changes
CI_JS_SUITES
andCI_NATIVE_SUITES
are now configurable, this makes itpossible to more easily run a coverage report against a subset of tests.
tools/test.py
now uses a black-list rather thana white-list; this makes it harder to miss adding a new test folder to CI.
known_issues
suite of tests is no longer run as part of coverage; myconcern being that we should count a line as covered, if it's exercised
by a failing test.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test,build
CC: @addaleax, @gibfahn @jasnell