FIX: hanging stream identification #189

[carlwilson]

• added simple test, with no assert, for file identification; and
• added similar for stream identification which demonstrates hang.

[carlwilson]

New commit added that fixes the loop and stops the tests failing.

[sevein]

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.

Suggested change

	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))

[carlwilson]

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.

[ross-spencer]

@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:

• Demos tmp_path like @sevein above: 18de5c0
• Brings in Pytest's capsys to allow us to consume the CSV and make assertions about it: 3b9e73f

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.

[carlwilson]

@adamfarquhar the work in this and #191 above might be useful for developing unit tests.

[carlwilson]

Closed as contained in #226

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.00%. Comparing base (cd85271) to head (96c3007).
Report is 39 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.