Improved handling of error conditions, logging, test ordering #6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 9 commits
April 29, 2015 10:54
If the number of workers requested is 1, then that's equivalent to running tests serially, so there's no reason to run them in a worker process with all of the overhead and extra log noise that entails. Therefore, disable the plugin when the number of requested workers is 1. Similarly, disable the plugin if the number of workers requested is the number of processor cores, and there's only one core available.
Add a gevented-timeout configuration setting to override the 60-second timeout for how long we will wait for individual test jobs to complete. When the results processor times out, print a list of the tests that took too long so the user can investigate why. If the results processor dies or exits because it timed out, stop sending new tasks to workers, because they're no point in sending tasks whose results will never be collected. If the remaining tasks list still has tasks in it when all the worker processes have exited, then display a warning listing the tasks for which no results were received, and exit immediately rather than blocking for 60 seconds (or whatever gevented-timeout is set to).
Time how long worker tasks take to execute and debug-log the execution times. Add --gevented-timing-file which, if specified, causes the plugin to store the task execution times for the last test run, and to sort the tasks from slowest to fastest before executing them the next time. This increases parallelization and therefore makes test runs faster. Incidental change: Collapse TasksQueueManager and TestsQueueManager into a single class; there was no justification whatsoever that I could see for keeping the separate TasksQueueManager class, which was used only by TestsQueueManager, and this artifical separation was was cluttering the code and making it more complicated.
The logger name should be nose.plugins.nose_gevented_multiprocess instead of nose_gevented_multiprocess, so that the standard logging level configuration for nose plugins will be applied to it properly.
When a particular test run doesn't execute a particular task, save the last calculated duration of that task in the gevented-timing-file. I debated whether to do this, because it means that if a test is deleted, stale data will persist for it in the state file until the state file is deleted. But that state takes very little space, and deleting tests happens much less frequently than running nosetests over a subset of the tests in a directory, so this seems like a net gain overall.
Rather than just logging a warning, raise an exception if we time out with remaining tasks. Otherwise, nosetests exits with a zero status even though the tests did not all succeed.
Change for compatibility with gevent 1.1, backward compatible to earlier gevent.
|
oh.. sorry. missed this PR in the noise of other work-related github notifications. I need to fix the event processing anyway, so will gladly look at merging thing. |
So 'gevented-processes=-2' means 'all but one', -3 means 'all but two', etc. Passing in a nonsense value means disabled.
after a "Timing out..." warning
instead of filtering out its siblings
not just the spawned shell, which left the client orphaned
Don't just hang there
We already cleaned up properly on a timeout in the results processor, but let's always clean up.
ENH: Clean up workers on exception, e.g. a KeyboardInterrupt
|
@richafrank @jikamens Added you to this repo as maintainers. I definitely play with this Python still, but not enough with this particular module to be a good single guardian of it. Please, ping me if I need to do anything special to do a release. Thx Guys |
|
Thanks @dvdotsenko . If you want to push a release to pypi that includes these changes, that'd be great - I think only you would have access to do that currently. (setup.py will also need updating for a version bump.) |
|
@richafrank What do I add to release notes? What's done / fixed in this merge? (Major vs Minor version release?) |
|
Hey @dvdotsenko , took a while to get back to this. Enhancements:
Fixes:
Regarding release number, how have you been versioning this so far? This diff cleans up lots of sharp edges and adds some small features, so I'd probably make it a minor release. Thankfully this project has nicely well-constrained scope, so I don't expect we'd see major changes often. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains a number of changes and enhancements, some minor and some significant. See the individual commit messages for details. Summary: