fox-it / flow.record

Recordization library
GNU Affero General Public License v3.0
7 stars 9 forks source link

Fix ValueError: I/O operation on closed file during tests #123

Closed yunzheng closed 3 months ago

yunzheng commented 4 months ago

Sometimes the stdout file object is closed by flow.record internals as it is sometimes mocked and swapped by pytest during tests which in turn can confuse is_stdout() to return False causing the file to be closed.

This is now fixed by adding two custom methods for getting the stdio streams:

These methods are the preferred way to get the stdio streams as they also set an extra attribute on the returned file object that is checked by is_stdout().

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.76%. Comparing base (0865b50) to head (ca4f7e1).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #123 +/- ## ========================================== + Coverage 83.72% 83.76% +0.04% ========================================== Files 34 34 Lines 3403 3413 +10 ========================================== + Hits 2849 2859 +10 Misses 554 554 ``` | [Flag](https://app.codecov.io/gh/fox-it/flow.record/pull/123/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fox-it) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/fox-it/flow.record/pull/123/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fox-it) | `83.76% <100.00%> (+0.04%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fox-it#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yunzheng commented 4 months ago

This sadly didn't fix the issue :(

Will leave this PR in Draft state for more adhoc tests.

yunzheng commented 3 months ago

Took a while but this should now be fixed. I took quite a deep dive into the pytest internals to figure out the cause.

It's mainly caused due to pytest's capture.py swapping sys.stdout with it's own objects (which is fine btw), however in our own code we check the file objects by comparing them to sys.stdout or sys.stdout.buffer, but as sys.stdout keeps getting swapped multiple times (eg: nested pytest operations/fixtures) it could be that a file object that was once a sys.stdout file is now not seen as stdout anymore, and therefore closed by flow.record internals at certain points.

The easiest fix was to ensure that when we request a stdout stream in flow.record to mark them as stdout ourselves, and ensure that they don't get closed.

I have no idea why this only happened on PyPy. Could be a race condition.