Silence stderr in tests #1708
Silence stderr in tests #1708
Conversation
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
✔️ Approved
Looks good, but in my local dev I still see the stacktraces. I'm not sure if that's intentional or not.
👀 @jotaen4tinypilot it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
✔️ Approved
In: Discussion
python -m unittest app.execute_test.ExecuteTest
...Process ProcessWithResult-2:
Traceback (most recent call last):
File "/Users/jason/.pyenv/versions/3.9.17/lib/python3.9/multiprocessing/process.py", line 315, in _bootstrap
self.run()
File "/Users/jason/repos/tinypilot/tinypilot/app/execute.py", line 37, in run
result.return_value = self._target(*self._args, **self._kwargs)
File "/Users/jason/repos/tinypilot/tinypilot/app/execute_test.py", line 32, in raise_exception
raise Exception('Child exception')
Exception: Child exception
...Process ProcessWithResult-5:
Traceback (most recent call last):
File "/Users/jason/.pyenv/versions/3.9.17/lib/python3.9/multiprocessing/process.py", line 315, in _bootstrap
self.run()
File "/Users/jason/repos/tinypilot/tinypilot/app/execute.py", line 37, in run
result.return_value = self._target(*self._args, **self._kwargs)
File "/Users/jason/repos/tinypilot/tinypilot/app/execute_test.py", line 32, in raise_exception
raise Exception('Child exception')
Exception: Child exception
...
----------------------------------------------------------------------
Ran 9 tests in 1.225s
OK👀 @jotaen4tinypilot it's your turn please take a look
|
(My response from CodeApprove doesn’t show up, so copying over for now.)
|
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
Oh, that’s weird… Did you run the tests on MacOS by any chance? I can confirm that I too see it when running on MacOS directly, but it looks alright in my Linux dev VM. I’m suspecting that this might be a Mac issue, similar (or related) to the forking vs. spawning thing.
I’ll give this a shot and see whether we can make it work on Mac in an easy and timely manner.
👀 @jdeanwallace it's your turn please take a look
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
Changes requested: I have approved this change on CodeApprove but some of my comments are unresolved.
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
In: Discussion
I’ve investigated this a little and dabbled around for a bit, but I wasn’t able to find a viable solution for MacOS. I think what we’d need to do is to somehow silence stderr inside the execute.py module (e.g. by redirecting to /dev/null), but then our fix would have a side-effect on production code, which I don’t think is a good trade-off.
Considering that this is only a somewhat minor (since stylistic) issue, local to the testing context, and specific to MacOS, I’d suggest to punt on this for the time being.
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
Approved: I have approved this change on CodeApprove and all of my comments have been resolved.
Resolves #1642: our test output is nice and tidy again.
Contrary to what I initially mentioned in the ticket, one of the two stacktraces didn’t come from
test_process_with_result_child_exception, which already had the “stderr silencing”, but rather fromtest_background_thread_ignores_function_exception. (I’ve updated the ticket accordingly.)The “stderr silencing” technique is working fine, we just didn’t have it on the other two functions. These were added later, so we likely just forgot about this issue.
I’ve extracted a context manager function, to make it easier to discover and re-use this technique in the tests, and to avoid us having to repeat the explanatory comment + exact invocation again and again.
