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

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 39 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,13 @@ If you are updating tests and just want to run a single test to check it:
$ python tools/test.py -J --mode=release parallel/test-stream2-transform
```

You can execute the entire suite of tests for a given subsystem
by providing the name of a subsystem:

```text
$ python tools/test.py -J --mode=release child-process
```

If you want to check the other options, please refer to the help by using
the `--help` option

Expand All @@ -420,6 +427,38 @@ $ ./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
Copy link
Contributor Author

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.


It's good practice to ensure any code you add or change is covered by tests.
Copy link
Member

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.

You can do so by running the test suite with coverage enabled:

```text
$ ./configure --coverage && make coverage
```
Copy link
Member

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.

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

Copy link
Member

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.


A detailed coverage report will be written to `coverage/index.html` for
JavaScript coverage and to `coverage/cxxcoverage.html` for C++ coverage.

_Note that generating a test coverage report can take several minutes._

To collect coverage for a subset of tests you can set the `CI_JS_SUITES` and
`CI_NATIVE_SUITES` variables:

```text
$ CI_JS_SUITES=child-process CI_NATIVE_SUITES= make coverage
```

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

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.

Copy link
Contributor Author

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.


Running tests with coverage will create and modify several directories
and files. To clean up afterwards, run:

```text
make coverage-clean
./configure && make -j4.
```

#### Step 7: Push

Once you are sure your commits are ready to go, with passing tests and linting,
Expand Down
20 changes: 15 additions & 5 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ TEST_CI_ARGS ?=
STAGINGSERVER ?= node-www
LOGLEVEL ?= silent
OSTYPE := $(shell uname -s | tr '[A-Z]' '[a-z]')
COVTESTS ?= test
COVTESTS ?= test-cov
GTEST_FILTER ?= "*"
GNUMAKEFLAGS += --no-print-directory

Expand Down Expand Up @@ -204,10 +204,20 @@ test: all
$(PYTHON) tools/test.py --mode=release -J \
$(CI_ASYNC_HOOKS) \
$(CI_JS_SUITES) \
$(CI_NATIVE_SUITES)
$(CI_NATIVE_SUITES) \
known_issues
$(MAKE) lint
endif

test-cov: all
$(MAKE) build-addons
$(MAKE) build-addons-napi
# $(MAKE) cctest
$(PYTHON) tools/test.py --mode=release -J \
$(CI_JS_SUITES) \
$(CI_NATIVE_SUITES)
$(MAKE) lint

test-parallel: all
$(PYTHON) tools/test.py --mode=release parallel -J

Expand Down Expand Up @@ -336,7 +346,7 @@ test-all-valgrind: test-build

CI_NATIVE_SUITES := addons addons-napi
CI_ASYNC_HOOKS := async-hooks
CI_JS_SUITES := abort doctool es-module inspector known_issues message parallel pseudo-tty sequential
CI_JS_SUITES ?= default

# Build and test addons without building anything else
test-ci-native: LOGLEVEL := info
Expand All @@ -349,7 +359,7 @@ test-ci-native: | test/addons/.buildstamp test/addons-napi/.buildstamp
test-ci-js: | clear-stalled
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES)
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) known_issues
# Clean up any leftover processes, error if found.
ps awwx | grep Release/node | grep -v grep | cat
@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \
Expand All @@ -362,7 +372,7 @@ test-ci: | clear-stalled build-addons build-addons-napi
out/Release/cctest --gtest_output=tap:cctest.tap
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES)
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) known_issues
# Clean up any leftover processes, error if found.
ps awwx | grep Release/node | grep -v grep | cat
@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \
Expand Down
54 changes: 29 additions & 25 deletions tools/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1531,23 +1531,6 @@ def ExpandCommand(args):
return prefix + args + suffix
return ExpandCommand


BUILT_IN_TESTS = [
'sequential',
'parallel',
'pummel',
'message',
'internet',
'addons',
'addons-napi',
'gc',
'debugger',
'doctool',
'inspector',
'async-hooks',
]


def GetSuites(test_root):
def IsSuite(path):
return isdir(path) and exists(join(path, 'testcfg.py'))
Expand All @@ -1566,6 +1549,32 @@ def PrintCrashed(code):
return "CRASHED (Signal: %d)" % -code


# these suites represent special cases that should not be run as part of the
# default JavaScript test-run, e.g., internet/ requires a network connection,
# addons/ requires compilation.
IGNORED_SUITES = [
'addons',
'addons-napi',
'gc',
'internet',
'pummel',
'test-known-issues',
'tick-processor',
'timers'
]


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


def Main():
parser = BuildOptions()
(options, args) = parser.parse_args()
Expand All @@ -1581,18 +1590,13 @@ def Main():
logger.addHandler(fh)

workspace = abspath(join(dirname(sys.argv[0]), '..'))
suites = GetSuites(join(workspace, 'test'))
test_root = join(workspace, 'test')
suites = GetSuites(test_root)
repositories = [TestRepository(join(workspace, 'test', name)) for name in suites]
repositories += [TestRepository(a) for a in options.suite]

root = LiteralTestSuite(repositories)
if len(args) == 0:
paths = [SplitPath(t) for t in BUILT_IN_TESTS]
else:
paths = [ ]
for arg in args:
path = SplitPath(NormalizePath(arg))
paths.append(path)
paths = ArgsToTestPaths(test_root, args, suites)

# Check for --valgrind option. If enabled, we overwrite the special
# command flag with a command that uses the run-valgrind.py script.
Expand Down
2 changes: 1 addition & 1 deletion vcbuild.bat
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ set enable_static=
set build_addons_napi=
set test_node_inspect=
set test_check_deopts=
set js_test_suites=abort async-hooks es-module inspector known_issues message parallel sequential
set js_test_suites=default async-hooks known_issues
set v8_test_options=
set v8_build_options=
set "common_test_suites=%js_test_suites% doctool addons addons-napi&set build_addons=1&set build_addons_napi=1"
Expand Down