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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Oct 16, 2018
Merged

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

merged 2 commits into from
Oct 16, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 3, 2018 •
edited
Loading

  • 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
Copy link
Collaborator

@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

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Oct 3, 2018
@refack refack added the python PRs and issues that require attention from people who are familiar with Python. label Oct 3, 2018
@refack refack requested a review from Trott October 3, 2018 23:54
@refack refack self-assigned this Oct 3, 2018
@refack refack force-pushed the teach-test.py-new-tricks branch from 92bae01 to 323234e Compare October 4, 2018 00:22
richardlau
richardlau reviewed Oct 4, 2018
View reviewed changes
test/root.status
@@ -0,0 +1,190 @@
[$mode==debug]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

addaleax commented Oct 4, 2018

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
Copy link
Contributor Author

refack commented Oct 4, 2018 •
edited
Loading

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
aqrln reviewed Oct 5, 2018
View reviewed changes
tools/test.py Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

aqrln
aqrln reviewed Oct 5, 2018
View reviewed changes
tools/test.py Outdated
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@refack refack force-pushed the teach-test.py-new-tricks branch from b459ac2 to b0adcab Compare October 5, 2018 21:55
@refack
Copy link
Contributor Author

refack commented Oct 5, 2018 •
edited
Loading

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/

aqrln
aqrln reviewed Oct 10, 2018
View reviewed changes
tools/test.py Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Oct 10, 2018

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
Copy link
Member

Trott commented Oct 10, 2018 •
edited by refack
Loading

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

@refack refack force-pushed the teach-test.py-new-tricks branch 2 times, most recently from 8e7acc8 to 453096c Compare October 10, 2018 17:23
@refack
Copy link
Contributor Author

refack commented Oct 10, 2018

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:

  1. Hoist common code to base class (GetTestStatus, and the section property to TestConfiguration)
  2. Replace ListSet with the built in set
  3. Remove ClassifiedTest
  4. Inline PrintReport
  5. 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
thefourtheye reviewed Oct 11, 2018
View reviewed changes
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 thought it's seems more readable this way (like a switch)...

tools/test.py Outdated Show resolved Hide resolved
tools/test.py Outdated Show resolved Hide resolved
@refack refack force-pushed the teach-test.py-new-tricks branch 2 times, most recently from 671520f to 41611a8 Compare October 11, 2018 17:06
@refack
Copy link
Contributor Author

refack commented Oct 11, 2018

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

@refack refack force-pushed the teach-test.py-new-tricks branch from 41611a8 to 172da2a Compare October 16, 2018 00:40
@refack
Copy link
Contributor Author

refack commented Oct 16, 2018

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/

refack added 2 commits October 16, 2018 11:09
@refack
tools,test: cleanup and dedup code
ec4f70e 
* 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 <[email protected]>
@refack
tools,test: add list of slow tests
deaddd2 
they will be skipped if run with `--flaky-tests=skip`

PR-URL: nodejs#23251
Reviewed-By: Rich Trott <[email protected]>
@refack refack force-pushed the teach-test.py-new-tricks branch from 172da2a to deaddd2 Compare October 16, 2018 15:10
@refack
Copy link
Contributor Author

refack commented Oct 16, 2018

landed in:
ec4f70e
deaddd2

@refack refack merged commit deaddd2 into nodejs:master Oct 16, 2018
@refack refack deleted the teach-test.py-new-tricks branch October 16, 2018 15:16
@rvagg rvagg mentioned this pull request Oct 17, 2018
Closed
3 tasks
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@refack @jasnell
tools,test: cleanup and dedup code
5b79d55 
* 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: #23251
Reviewed-By: Rich Trott <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@refack @jasnell
tools,test: add list of slow tests
74c4bb7 
they will be skipped if run with `--flaky-tests=skip`

PR-URL: #23251
Reviewed-By: Rich Trott <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@refack @addaleax
tools,test: cleanup and dedup code
ab9527d 
* 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: #23251
Reviewed-By: Rich Trott <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@refack @addaleax
tools,test: add list of slow tests
2b863f0 
they will be skipped if run with `--flaky-tests=skip`

PR-URL: #23251
Reviewed-By: Rich Trott <[email protected]>
@refack refack removed their assignment Oct 20, 2018
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@refack @MylesBorins
tools,test: cleanup and dedup code
148ae30 
* 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: #23251
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@refack @MylesBorins
tools,test: add list of slow tests
9e03393 
they will be skipped if run with `--flaky-tests=skip`

PR-URL: #23251
Reviewed-By: Rich Trott <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Nov 23, 2018
Closed
4 tasks
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@refack @rvagg
tools,test: cleanup and dedup code
8a28568 
* 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: #23251
Reviewed-By: Rich Trott <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@refack @rvagg
tools,test: add list of slow tests
d96c807 
they will be skipped if run with `--flaky-tests=skip`

PR-URL: #23251
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@refack @MylesBorins
tools,test: cleanup and dedup code
8e08a27 
* 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: #23251
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@refack @MylesBorins
tools,test: add list of slow tests
e1f7924 
they will be skipped if run with `--flaky-tests=skip`

PR-URL: #23251
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
@richardlau richardlau mentioned this pull request Dec 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@aqrln aqrln aqrln left review comments

@thefourtheye thefourtheye thefourtheye left review comments

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
python PRs and issues that require attention from people who are familiar with Python. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@refack @nodejs-github-bot @addaleax @Trott @thefourtheye @aqrln @richardlau