FIX: hanging stream identification #189 #190
Conversation
carlwilson
commented
May 14, 2020
carlwilson
commented
- added simple test, with no assert, for file identification; and
- added similar for stream identification which demonstrates hang.
- added simple test, with no assert, for file identification; and - added similar for stream identification which demonstrates hang.
carlwilson
added
bug
- fixed termination condition in `blocking_read`.
|
New commit added that fixes the loop and stops the tests failing. |
| def test_file_identification(): | ||
| """Reference for Fido-based format identification | ||
| 1. Create a byte-stream with a known magic number and serialise to tempfile. | ||
| 2. Call identify_file(...) to identify the file against Fido's known formats. | ||
| """ | ||
| # Create a temporary file on the host operating system. | ||
| tmp = tempfile.mkstemp() | ||
| tmp_file = tmp[1] | ||
|
|
||
| # Write to the file our known magic-number. | ||
| with open(tmp_file, "wb") as new_file: | ||
| new_file.write(MAGIC) | ||
|
|
||
| # Create a Fido instance and call identify_file. The identify_file function | ||
| # will create and manage a file for itself. | ||
| f = fido.Fido() | ||
| f.identify_file(tmp_file) |
There was a problem hiding this comment.
Nice tests, Carl. I think this one could be a good use case for the tmp_path fixture provided by pytest. It gives us a pathlib.Path object which has a nice API. pytest also takes care of removing old temporary directories, always keeping the last three.
| def test_file_identification(): | |
| """Reference for Fido-based format identification | |
| 1. Create a byte-stream with a known magic number and serialise to tempfile. | |
| 2. Call identify_file(...) to identify the file against Fido's known formats. | |
| """ | |
| # Create a temporary file on the host operating system. | |
| tmp = tempfile.mkstemp() | |
| tmp_file = tmp[1] | |
| # Write to the file our known magic-number. | |
| with open(tmp_file, "wb") as new_file: | |
| new_file.write(MAGIC) | |
| # Create a Fido instance and call identify_file. The identify_file function | |
| # will create and manage a file for itself. | |
| f = fido.Fido() | |
| f.identify_file(tmp_file) | |
| def test_file_identification(tmp_path): | |
| """Reference for Fido-based format identification | |
| 1. Create a byte-stream with a known magic number and serialise to tempfile. | |
| 2. Call identify_file(...) to identify the file against Fido's known formats. | |
| """ | |
| # Create a temporary file on the host operating system. | |
| tmp_file = tmp_path / "file" | |
| # Write to the file our known magic-number. | |
| tmp_file.write_bytes(MAGIC) | |
| # Create a Fido instance and call identify_file. The identify_file function | |
| # will create and manage a file for itself. | |
| f = fido.Fido() | |
| f.identify_file(str(tmp_file)) |
There was a problem hiding this comment.
So the original ideas here came from @ross-spencer (that's intended to give credit not share blame for temp file management). I agree that these could be better for test purposes. The PR will remain draft for now as I'm going to test a couple of ideas around the API.
|
@carlwilson I hadn't seen @sevein's inline suggestion when I put this on my todo list. I've only just registered this coming back to it tonight but I had already created a branch and PR for you to take a look at here, which does the following too:
They might be good enough for you to merge, but maybe also develop out into some negative FIDO result testing too, i.e. negative identifications, if you have time/interest which it seems like you do (interest anyway, if not heaps of time! 😉 ) There was a build warning I noticed late on and corrected as well in that series of commits. Anyhow, I hope this work is a useful platform and not an annoyance at all. I just happened to have time at the end of my day today to take a look. |
|
@adamfarquhar the work in this and #191 above might be useful for developing unit tests. |
|
Closed as contained in #226 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
===========================================
+ Coverage 23.42% 34.00% +10.58%
===========================================
Files 11 11
Lines 1490 1491 +1
===========================================
+ Hits 349 507 +158
+ Misses 1141 984 -157 ☔ View full report in Codecov by Sentry. |
