-
Notifications
You must be signed in to change notification settings - Fork 5
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
Expose the --buffer setting for the parallel test runner too #183
base: main
Are you sure you want to change the base?
Conversation
result_handler = ChildResultHandler() | ||
result_collector = ResultCollector(buffer=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, that I don't fully understand the usage of ResultCollector
, and in particular why its instantiation happens inside _run_test_in_process()
. This is a simple attempt at solving issue #182.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The currentl implementation is very similar to setting buffer=False
which results in output from the test going to the parent sys.stdout and sys.stderr. So the rest of the changes are not necessary.
Yet, not buffering the test output is not really a correct solution to the issue #182. The main problem is that test executed in parallel will write into the parent stdout/stderr streams at the same time making the results unreadable.
The correct solution is to buffer the output for each test and print it out in the parent process when the result has been collected. This way the output for each test will be selfcontained and readable.
Unfortunately, at the moment while both buffer stdout and stderr streams are created (i.e.buffer=True
). When the results are accumulated, the stdout/stderr content is ignored unless there was a test exception/failure (see https://github.com/scalative/haas/blob/master/haas/result.py#L249). I think that fixing #182 will invlove fixing the behaviour (and maybe design) of the TestResult class.
Thanks @pzahemszky. I will have a look over the weekend. |
Thank you, Ioannis. Do you happen to know if the Python 2.6 and 3.3 jobs can be removed? They are failing on |
The python 2.6 and 3.3 failures are not related to this PR and we need to investigate before taking any action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation is a limited fix of the bug, so I cannot recommend that it is merged.
result_handler = ChildResultHandler() | ||
result_collector = ResultCollector(buffer=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The currentl implementation is very similar to setting buffer=False
which results in output from the test going to the parent sys.stdout and sys.stderr. So the rest of the changes are not necessary.
Yet, not buffering the test output is not really a correct solution to the issue #182. The main problem is that test executed in parallel will write into the parent stdout/stderr streams at the same time making the results unreadable.
The correct solution is to buffer the output for each test and print it out in the parent process when the result has been collected. This way the output for each test will be selfcontained and readable.
Unfortunately, at the moment while both buffer stdout and stderr streams are created (i.e.buffer=True
). When the results are accumulated, the stdout/stderr content is ignored unless there was a test exception/failure (see https://github.com/scalative/haas/blob/master/haas/result.py#L249). I think that fixing #182 will invlove fixing the behaviour (and maybe design) of the TestResult class.
This pull request makes the command-line
--buffer
setting take effect for the parallel test runner too. Since it's off by default, the parallel runner will display the contents of the standard output (and those of the standard error) unless asked not to. Fixes #182.Using the test_haas_parallel.py example script from ticket #182: