Skip to content
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

Python: test on different writable streams #98

Open
wants to merge 5 commits into
base: serialization
Choose a base branch
from

Conversation

generalmimon
Copy link
Member

So far, the generic test_read_write_roundtrip test and some specific write tests for EOF errors were only testing on the memory-backed BytesIO stream. This is fine in most cases, but it turns out that sometimes it's necessary to test on specific file streams instead, because they have slightly different behavior in certain cases, as evidenced by these issues:

Neither of these issues can be reproduced with BytesIO, but they are easy to reproduce with the changes in this PR.

This PR replaces one test_read_write_roundtrip test with several tests, each using a different stream. In Python 3, the built-in open() and the io.open() functions are equal (at least in CPython, they even both refer to the same function object, i.e. (open is io.open) == True, which I also use in the test code), whereas in Python 2, open() and io.open() are different and they also return streams with slightly different API and behavior of the provided methods.

So these are the new roundtrip test methods in Python 3:

  • test_read_write_roundtrip__InMemoryStream
  • test_read_write_roundtrip__TemporaryFile_io_open_rdwr
  • test_read_write_roundtrip__TemporaryFile_io_open_rdwr_nobuf
  • test_read_write_roundtrip__TemporaryFile_io_open_wronly
  • test_read_write_roundtrip__TemporaryFile_io_open_wronly_nobuf

and Python 2 has the same methods plus these:

  • test_read_write_roundtrip__TemporaryFile_builtin_open_rdwr
  • test_read_write_roundtrip__TemporaryFile_builtin_open_rdwr_nobuf
  • test_read_write_roundtrip__TemporaryFile_builtin_open_wronly
  • test_read_write_roundtrip__TemporaryFile_builtin_open_wronly_nobuf

InMemoryStream and TemporaryFile refer to the names of helper classes in spec/python/extra/helpers.py. InMemoryStream is a wrapper for io.BytesIO, TemporaryFile creates (using tempfile.mkstemp()) and manages a temporary file.


There is also a helper class Pipe for wrapping a pipe provided by the OS, but it's currently not used. Current implementation of serialization relies on seeking quite a bit - even substreams require seeking support, EOF checks rely on seeking (even though we could skip them in case seeking doesn't work), etc. Therefore, the Python runtime raises ValueError("writing to non-seekable streams is not supported") when there's an attempt to write to a non-seekable stream. However, testing on pipes would make sense for parsing, where the only features requiring seeking should be instances and consume: false. This is left as a future improvement.

@generalmimon
Copy link
Member Author

These commits fixed runtime library bugs detected by the newly introduced tests: kaitai-io/kaitai_struct_python_runtime@ebfae5f...28de847

@generalmimon generalmimon requested a review from GreyCat July 22, 2023 15:25
@generalmimon
Copy link
Member Author

generalmimon commented Jul 22, 2023

@GreyCat I'm pretty confident that these new tests work as intended (and I've already reworked the implementation several times to make it as simple as possible, but I'm relatively happy with it now), but I'll leave this PR open for a while in case you want to take a look.

@generalmimon generalmimon force-pushed the python-streams-with-different-capabilities branch from 77090c3 to 136b9e6 Compare July 22, 2023 19:58
@generalmimon generalmimon force-pushed the python-streams-with-different-capabilities branch from 136b9e6 to 91c81f1 Compare July 27, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant