test: make it easier to run tests for subsystems #15450

bcoe · 2017-09-17T20:07:38Z

Problem statement

it would be nice to be able to more easily run tests across a given
subsystem, e.g., http, when their tests are spread out across
multiple folders (pummel/, sequential/, etc.).
it would be nice to be able to report on coverage for a subset of
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:

you end up changing the git-history of 1000s of files.
at a glance, there's even more of a paradox of choice with regards
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` configurable

Rather than reorganizing tests into an alternative folder structure, this
pull request introduces the concept of a suites_<subsystem> shorthand to
test.py.

Running:

$ python tools/test.py -J --mode=release suites_child-process

Would result in any tests matching the */test-child-process-* pattern being
executed, regardless of whether they are contained in test/sequential,
test/parallel, etc.

Other changes

CI_JS_SUITES and CI_NATIVE_SUITES are now configurable, this makes it
possible to more easily run a coverage report against a subset of tests.
the default suites run by tools/test.py now uses a black-list rather than
a white-list; this makes it harder to miss adding a new test folder to CI.
the known_issues suite of tests is no longer run as part of coverage; my
concern being that we should count a line as covered, if it's exercised
by a failing test.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test,build

CC: @addaleax, @gibfahn @jasnell

bcoe · 2017-09-17T20:09:45Z

CONTRIBUTING.md

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


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

addaleax · 2017-09-17T20:13:03Z

CONTRIBUTING.md

+$ make coverage
+```
+
+A detailed coverage report will be output to `coverage/index.html`.


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?

addaleax · 2017-09-17T20:16:03Z

tools/test.py

+        if suite == 'default':
+          paths += default_suites
+        else:
+          path = SplitPath(NormalizePath("*/test-%s-*" % (suite)))


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

Also, this approach feels a bit like it might be too much magic :) Not sure what to do about that, though.

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?

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?

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

What you said sounds reasonable, but probably worth waiting to see what others think.

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

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

refack · 2017-09-17T22:28:06Z

I had an idea floating in my head to split the test files (at least in parallel) into "sub-suites", i.e. /test/parallel/assert, /test/parallel/async-hooks, etc.
Then for this kind of cross folder "sub-system" triggering you could use special a special char like * so you could do any of the follow:

$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

refack · 2017-09-17T22:28:33Z

/cc @nodejs/testing

Trott · 2017-09-18T03:23:08Z

CONTRIBUTING.md

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


Nit: no comma after coverage

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

bnoordhuis · 2017-09-18T11:36:19Z

CONTRIBUTING.md

+`CI_NATIVE_SUITES` variables:
+
+```text
+$ CI_JS_SUITES=suites_child-process CI_NATIVE_SUITES= make coverage


$ make coverage CI_JS_SUITES=suites_child-process CI_NATIVE_SUITES= - also works with shells that don't support VAR=value cmd syntax.

I didn't know they existed, which shells are those?

At least csh and tcsh don't. (Not that I expect many people use those but still.)

bnoordhuis · 2017-09-18T11:37:38Z

tools/test.py

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


"These" (capital)

bnoordhuis · 2017-09-18T11:38:21Z

tools/test.py

+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):


The paren are not necessary around the not in expression.

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}

refack · 2017-09-18T22:34:41Z

tools/test.py

  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)


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

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.

Ohh I swallowed that...
I think restoring it here is also good.

[updated snippet above]

bcoe · 2017-09-23T00:38:36Z

@refack @Trott I believe I've addressed your issues.

Tests will not pass on Windows for this branch until we land #15490, which fixes es-modules on Windows.

Trott · 2017-09-23T03:55:47Z

CONTRIBUTING.md

+```
+
+The above command executes tests for the `child-process` subsystem and
+outputs the resulting coverage report.


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.

No worries, appreciate the thorough review. I just pushed your recommendations, let me know what you think and I'll rebse.

Trott · 2017-09-23T03:56:35Z

CONTRIBUTING.md

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


Nit: Add sentence here along these lines:

Note that generating a test coverage report can take several minutes.

Trott · 2017-09-23T03:57:56Z

CONTRIBUTING.md

+
+```text
+$ ./configure --coverage && make coverage
+```


Nit: Maybe add something like this?

You may see tests failing that normally succeed when not generating a coverage report. Do not be alarmed.

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.

I agree with @bcoe , I'd rather we treated coverage test failures as bugs if possible.

Trott · 2017-09-23T04:01:00Z

CONTRIBUTING.md

+$ ./configure --coverage && make coverage
+```
+
+A detailed coverage report will be output to `coverage/index.html` for


Micro-nit: will be output to -> will be written to

Trott · 2017-09-23T04:01:26Z

Left more nits but I don't consider any of them blocking.

BridgeAR · 2017-09-24T17:15:40Z

CI https://ci.nodejs.org/job/node-test-pull-request/10240/

BridgeAR · 2017-09-24T18:33:34Z

Seems like there are more errors in the es-module.

https://ci.nodejs.org/job/node-test-commit-smartos/11651/nodes=smartos16-64/console

not ok 1844 es-module/test-esm-pkg-over-ext
  ---
  duration_ms: 0.428
  severity: fail
  stack: |-
    (node:48485) ExperimentalWarning: The ESM module loader is experimental.
    Error: module ../fixtures/module-pkg-over-ext/inner not found
        at module.exports (internal/loader/search.js:16:12)
        at resolveRequestUrl (internal/loader/resolveRequestUrl.js:81:13)
        at Loader.getModuleJob (internal/loader/Loader.js:50:27)
        at ModuleWrap.module.link (internal/loader/ModuleJob.js:30:25)
        at linked (internal/loader/ModuleJob.js:28:21)

bcoe · 2017-09-24T19:57:41Z

@BridgeAR this is addressed in:

#15276, which I will rebase with the Windows URL fixes so that its tests start running in prod.

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.

bcoe · 2017-09-29T01:32:35Z

@BridgeAR @jasnell okay, this is rebased with the Windows path fixes and the pull that removed the failing es-module test ... tldr; should be able to kick off ci.

TimothyGu · 2017-09-29T01:37:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10326/

jasnell · 2017-09-29T16:45:30Z

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?

Trott · 2017-09-29T20:53:01Z

Running CI again to see if the same types of failures recur. (They're not on master. I just ran a node-daily-master to confirm that.)

CI: https://ci.nodejs.org/job/node-test-pull-request/10343/

bcoe · 2017-09-29T21:11:37Z

@Trott @jasnell I added tick-processor to the IGNORED_SUITES list, on the failing linux builds I saw this suite of tests appeared to be the problem. It would not have been being run using the old approach:

abort doctool es-module inspector known_issues message parallel pseudo-tty sequential

fun future work, try to get some of these flaky suites working on all platforms.

Trott · 2017-09-29T22:53:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10347/

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

BridgeAR · 2017-10-01T23:31:45Z

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: nodejs/node#15450 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

mhdawson · 2017-10-10T19:37:05Z

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 ?

refack · 2017-10-10T23:26:08Z

#16132 restores some suites back for the test-ci-js target

MylesBorins · 2017-10-11T06:14:18Z

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

MylesBorins · 2017-11-14T20:56:49Z

This does not land cleanly on v6.x does it make sense to backport?

if so please include #16132

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Sep 17, 2017

bcoe commented Sep 17, 2017

View reviewed changes

addaleax approved these changes Sep 17, 2017

View reviewed changes

Trott reviewed Sep 18, 2017

View reviewed changes

bnoordhuis reviewed Sep 18, 2017

View reviewed changes

refack reviewed Sep 18, 2017

View reviewed changes

bcoe force-pushed the test-refactor-pass-2 branch from 29a057a to c6901c0 Compare September 23, 2017 00:36

Trott reviewed Sep 23, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

bcoe force-pushed the test-refactor-pass-2 branch from 036a027 to 04b90f1 Compare September 29, 2017 01:22

bcoe force-pushed the test-refactor-pass-2 branch from 04b90f1 to 666f20e Compare September 29, 2017 01:31

test: ignore the tick-processor suite

da4e2d6

BridgeAR closed this Oct 1, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

mhdawson mentioned this pull request Oct 10, 2017

Coverage drop in nightly runs #16129

Closed

MylesBorins added the backport-requested-v6.x label Nov 14, 2017

test: make it easier to run tests for subsystems #15450

test: make it easier to run tests for subsystems #15450

Conversation

bcoe commented Sep 17, 2017 • edited Loading

Problem statement

Potential solution: split tests into subsystem-specific folders

Proposed solution: introduce suites_<subsystem> shorthand, make CI_JS_SUITES CI_NATIVE_SUITES configurable

Other changes

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 17, 2017

refack commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

bcoe Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

refack Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

bcoe commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Sep 23, 2017

BridgeAR commented Sep 24, 2017

BridgeAR commented Sep 24, 2017

bcoe commented Sep 24, 2017

bcoe commented Sep 29, 2017

TimothyGu commented Sep 29, 2017

jasnell commented Sep 29, 2017

Trott commented Sep 29, 2017

bcoe commented Sep 29, 2017

Trott commented Sep 29, 2017

BridgeAR commented Oct 1, 2017

mhdawson commented Oct 10, 2017

refack commented Oct 10, 2017

MylesBorins commented Oct 11, 2017 • edited Loading

MylesBorins commented Nov 14, 2017

bcoe commented Sep 17, 2017 •

edited

Loading

Proposed solution: introduce suites_<subsystem> shorthand, make `CI_JS_SUITES` `CI_NATIVE_SUITES` configurable

gibfahn Sep 17, 2017 •

edited

Loading

bcoe Sep 17, 2017 •

edited

Loading

refack Sep 18, 2017 •

edited

Loading

bcoe Sep 18, 2017 •

edited

Loading

refack Sep 18, 2017 •

edited

Loading

MylesBorins commented Oct 11, 2017 •

edited

Loading