-
Notifications
You must be signed in to change notification settings - Fork 409
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
[MRG] Replace stdout / stderr switching in smoke tests with capsys. #459
Conversation
kd@dragonstone:~/Documents/joblib$ git grep sys.stdout joblib/test/test*.py
joblib/test/test_memory.py: sys.stdout.write('Reloading\n')
joblib/test/test_testing.py: 'sys.stdout.flush()',
joblib/test/test_testing.py: 'sys.stdout.flush()'])
kd@dragonstone:~/Documents/joblib$ git grep sys.stderr joblib/test/test*.py
joblib/test/test_testing.py: 'sys.stderr.write("writing on stderr")',
joblib/test/test_testing.py: 'sys.stderr.write("before sleep on stderr")',
joblib/test/test_testing.py: 'sys.stderr.flush()', Only these occurences remain in test files. |
f(1) | ||
|
||
# Now reload | ||
sys.stdout.write('Reloading\n') |
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.
@lesteve Is it appropriate to use print
here ?
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.
sys.stdout.write is fine.
finally: | ||
sys.stdout = orig_stdout | ||
sys.stderr = orig_stderr | ||
for verbose in (2, 11, 100): |
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.
Hmmm interesting, so the captured stdout was not used at all, right? I am wondering why it was done like this.
I am wondering whether it would be worth doing some testing of the Parallel logging. Maybe not in this test though.
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.
@lesteve I am getting lines like:
[Parallel(n_jobs=?)]: Done ? out of ? | elapsed: ?.?s remaining: ?.?s
where ?
is a digit.
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.
I was saying this more for another PR and this is not super high priority, but you could add a test function that would check (bunch of random ideas, not all of them are worth pursuing):
- verbose=0 does not print anything
- n_jobs is set correctly in the printout
- done is > 0 and growing
- elapsed is growing
- remaining is never negative (we had a bug like this at some point) and maybe decreasing (not sure about that it could well be some oscillations)
- with increasing verbose you get more and more lines in your output
Not super high priority but nice to have and fun to write ;-).
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.
Noted for a future PR. 😉
LGTM, merging thanks! |
Fixes #457
Since joblib now uses pytest instead of nose, one does not need to explicitly capture the output. In many tests,
sys.stdout
andsys.stderr
were temporarily replaced by a dummyStringIO
ortempfile
, which were restored back after test execution completed.This manual switching is now removed owing to default behaviour of pytest (refer #457 description). Pytest's
capsys
fixture is used to grab the output and perform regex matching.