Python: test on different writable streams#98
Open
generalmimon wants to merge 5 commits intokaitai-io:serializationfrom
Open
Python: test on different writable streams#98generalmimon wants to merge 5 commits intokaitai-io:serializationfrom
generalmimon wants to merge 5 commits intokaitai-io:serializationfrom
Conversation
Member
Author
|
These commits fixed runtime library bugs detected by the newly introduced tests: kaitai-io/kaitai_struct_python_runtime@ebfae5f...28de847 |
Member
Author
|
@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. |
77090c3 to
136b9e6
Compare
136b9e6 to
91c81f1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
So far, the generic
test_read_write_roundtriptest and some specific write tests for EOF errors were only testing on the memory-backedBytesIOstream. 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_roundtriptest with several tests, each using a different stream. In Python 3, the built-inopen()and theio.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()andio.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__InMemoryStreamtest_read_write_roundtrip__TemporaryFile_io_open_rdwrtest_read_write_roundtrip__TemporaryFile_io_open_rdwr_nobuftest_read_write_roundtrip__TemporaryFile_io_open_wronlytest_read_write_roundtrip__TemporaryFile_io_open_wronly_nobufand Python 2 has the same methods plus these:
test_read_write_roundtrip__TemporaryFile_builtin_open_rdwrtest_read_write_roundtrip__TemporaryFile_builtin_open_rdwr_nobuftest_read_write_roundtrip__TemporaryFile_builtin_open_wronlytest_read_write_roundtrip__TemporaryFile_builtin_open_wronly_nobufInMemoryStreamandTemporaryFilerefer to the names of helper classes inspec/python/extra/helpers.py.InMemoryStreamis a wrapper forio.BytesIO,TemporaryFilecreates (usingtempfile.mkstemp()) and manages a temporary file.There is also a helper class
Pipefor 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 raisesValueError("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 beinstancesandconsume: false. This is left as a future improvement.