tools,test: teach test.py new tricks #23251

refack · 2018-10-03T23:54:08Z

refactor and deduplicate code
add list of slow tests - to be skipped for our CI debug build

The target is to get a regular CI job to finish in <= 1h (currently the debug build takes the longest to run).
A more extensive suite will run only once a day.

/CC @nodejs/python @nodejs/testing

~~CI: https://ci.nodejs.org/job/node-test-pull-request/17613/~~
CI: https://ci.nodejs.org/job/node-test-pull-request/17614/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-10-03T23:54:10Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1118/pipeline

richardlau · 2018-10-04T03:12:22Z

test/root.status

@@ -0,0 +1,190 @@
+[$mode==debug]


I'd rather keep the test status in the respective .status files in the test directories (e.g. parallel.status).

How strongly do you feel about that? IMHO having it in one place (1) will enabled easier auditing of this list (2) expresses that it's a cross-cutting concern.

I could instead move all the other rules to this single place, that also has benefits.

Not that strongly 😆.

I could instead move all the other rules to this single place, that also has benefits.

I could get behind this. My worry is that at some point other annotations (e.g. flaky ) start appearing in root.status in addition to e.g. parallel.status (where they currently are set) and I'd rather avoid any potential confusion.

What do @nodejs/testing think?

addaleax · 2018-10-04T05:05:43Z

A more extensive suite will run only once a day.

Just to clarify: This only affects test in debug mode, and only when run in CI, and we still run the full suite in debug mode every day? If yes, 👍 good idea!

Also, could you maybe write down what metric you used to tell slow tests apart? We’d probably want to move them off the list if we improve their performance.

refack · 2018-10-04T14:26:17Z

Just to clarify: This only affects test in debug mode

yes.

and only when run in CI

I'll verify.

and we still run the full suite in debug mode every day?

Yes.

Also, could you maybe write down what metric you used to tell slow tests apart? We’d probably want to move them off the list if we improve their performance.

I copied an (arbitrary) test result to a spreadsheet, and selected all tests that took > 10s:

Real job runtime was: 1 hr 38 min
Aggregated time (due to parallelism): ~16,600s
Longest (> 10s) 190 tests took: ~6700s (40%)
another interesting data point *benchmark* tests: 1400s (%8.5)

P.S. debug CI job for this PR - 57m

In general a well ccached job will only take 7m to compile.

P.P.S. comparison with job 7574 - https://www.diffchecker.com/5I31a6oR (50 tests mismatch)

aqrln · 2018-10-05T10:41:24Z

tools/test.py


  if options.time:
    # Write the times to stderr to make it easy to separate from the
    # test output.
-    print
+    print()


This will output an empty tuple in Python 2 (unless from __future__ import print_function).

ack.
Since it's just to add a new line I'd changed it to print('')

aqrln · 2018-10-05T15:18:44Z

tools/test.py

-  cases_to_run = [ c for c in all_cases if not DoSkip(c) ]
+      return False
+    return not (FLAKY in case.outcomes and options.flaky_tests == SKIP)
+  cases_to_run = filter(DoKeep, all_cases)


Since you're trying to make the script compatible with both Python 2 and Python 3, I don't think this change makes sense. A list comprehension would work identically in either version, but filter is an iterator in Python 3, so it would raise an exception a few lines below at https://github.com/nodejs/node/pull/23251/files#diff-37479d55e637ac7befeefff7f6d88025R1748.

Yeah, I should "pick a lane"... Either new functionality, or 2to3...
I'll give this whole PR another pass. Thanks for the review!

refack · 2018-10-05T21:59:37Z

I've refactored this PR into two commits
f89fd3b - Pure refactoring, deduping, and delinting
b0adcab - Teach test.py to read test/root.status and to skip slow tests when --flaky-tests=skip

PTAL

CI: https://ci.nodejs.org/job/node-test-pull-request/17651/
Resume: https://ci.nodejs.org/job/node-test-pull-request/17655/

tools/test.py

Trott · 2018-10-10T16:47:08Z

This seems good to me, but should it be two different PRs to ease review? Like, I feel comfortable signing off on one of the commits, but want to be a bit more careful reviewing the other one. If you're OK with being patient, I can spend some time with this to review more thoroughly if other people don't come along and ✔️ it soon...

Trott · 2018-10-10T16:49:27Z

Fresh CI: ~~https://ci.nodejs.org/job/node-test-pull-request/17723/~~
https://ci.nodejs.org/job/node-test-pull-request/17725/

refack · 2018-10-10T17:27:35Z

This seems good to me, but should it be two different PRs to ease review?

I reduced the amount of refactoring in the first commit (2b24387). But in order to keep the code for the new feature succinct, I decided to keep four changes:

Hoist common code to base class (GetTestStatus, and the section property to TestConfiguration)
Replace ListSet with the built in set
Remove ClassifiedTest
Inline PrintReport
How cases_to_run are filtered

plus a few trivial changes (non shadowing names, more built in idioms)

The second commit (453096c) has the code needed to skip SLOW tests when called with --flaky-tests=skip

thefourtheye · 2018-10-11T12:27:56Z

tools/test.py

+  def should_keep(case):
+    if any([ (s in case.file) for s in options.skip_tests ]):
+      return False
+    elif SKIP in case.outcomes:


Nit: Since all the branches return, these could be simply ifs

I thought it's seems more readable this way (like a switch)...

tools/test.py

refack · 2018-10-11T17:06:56Z

CI: https://ci.nodejs.org/job/node-test-commit/22176/

refack · 2018-10-16T01:01:51Z

Just debug: https://ci.nodejs.org/job/node-test-commit-linux-containered/7849/nodes=ubuntu1604_sharedlibs_debug_x64
Full CI: https://ci.nodejs.org/job/node-test-pull-request/17884/

* Hoist common code to base class (`GetTestStatus`, and the `section` property to `TestConfiguration`) * Replace ListSet with the built in set * Remove ClassifiedTest * Inline PrintReport * How cases_to_run are filtered PR-URL: nodejs#23251 Reviewed-By: Rich Trott <rtrott@gmail.com>

they will be skipped if run with `--flaky-tests=skip` PR-URL: nodejs#23251 Reviewed-By: Rich Trott <rtrott@gmail.com>

refack · 2018-10-16T15:11:39Z

landed in:
ec4f70e
deaddd2