Skip to content

qa: Remove polling loop from test_runner #13384

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
Closed
wants to merge 1 commit into from
Closed

qa: Remove polling loop from test_runner #13384

wants to merge 1 commit into from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jun 3, 2018

My initial design for parallel tests was a busy loop (busy waiting) that polls the running processes every half a second.

Using pythons own ThreadPoolExecutor makes the code smaller and hopefully easier to read/understand. By removing the half-a-second overhead, I believe the tests run now slightly faster (Verified on my workstation, but should also be verified by one of the reviewers, ideally)

This is refactoring beside the following changes:

@maflcko maflcko added the Tests label Jun 3, 2018
@maflcko maflcko force-pushed the Mf1806-qaTestRunnerConcurrentFuture branch 2 times, most recently from facf1a0 to fa30a1d Compare June 3, 2018 18:44
@maflcko maflcko force-pushed the Mf1806-qaTestRunnerConcurrentFuture branch from fa30a1d to fa35a02 Compare June 3, 2018 19:26
@laanwj
Copy link
Member

laanwj commented Jun 4, 2018 •
edited
Loading

I don't see difference with regard to run time:

with
real    3m54.986s
real    3m56.017s
real    4m0.519s
real    3m45.475s
real    3m57.155s

without
real    3m51.195s
real    3m54.101s
real    3m53.719s
real    4m1.334s
real    3m53.727s

However I like the code cleanup and it doesn't make it worse so utACK fa35a02

@laanwj
Copy link
Member

laanwj commented Jun 7, 2018

Seems that with this change, Ctrl-C does no longer quit the test runner immediately when the tests have started, looks like it has to be pressed twice.

Dropped the dot-printing (....) while waiting for the next result

I suddenly remember that this was added to prevent Travis from timing out due to no output. Is that no longer a concern?

@maflcko
Copy link
Member Author

maflcko commented Jun 7, 2018

I suddenly remember that this was added to prevent Travis from timing out due to no output. Is that no longer a concern?

The run time is between 2 and 3 minutes. And travis would only time out after 10 minutes of no dots received. The extended tests are not run on travis for a couple of weeks now, so I can't answer that...

@maflcko maflcko closed this Jun 7, 2018
@maflcko maflcko deleted the Mf1806-qaTestRunnerConcurrentFuture branch June 7, 2018 15:21
@maflcko
Copy link
Member Author

maflcko commented Jun 7, 2018

Closing for now. Not really worth the effort

@jnewbery
Copy link
Contributor

jnewbery commented Jun 8, 2018

Concept ACK. I'd considered replacing our custom-made process pool with multiprocessing.pool.Pool() previously.

Even if you don't include that change, it seems like some of the tidy-up in this PR is useful (eg the port_offset stuff)

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maflcko @laanwj @jnewbery