Converting tests to pytests discovers duplicated test code

[eumiro]

During the attempt to convert the test suite to PyTest, I have discovered, that the two tests:

• test_10_passing_file_object
• test_20_passing_filename

from

destream/tests/test_10_base.py

Lines 50 to 64 in 00f9fab

	def test_10_passing_file_object(self):
	text = b"Hello World!\n"
	with TemporaryFile() as fileobj:
	fileobj.write(text)
	fileobj.flush()
	archive = ArchiveFile(fileobj=fileobj)
	self._regular_tests(archive, fileobj, fileobj.name, text)
	def test_20_passing_filename(self):
	text = b"Hello World!\n"
	with TemporaryFile() as fileobj:
	fileobj.write(text)
	fileobj.flush()
	archive = ArchiveFile(fileobj=fileobj)
	self._regular_tests(archive, fileobj, fileobj.name, text)

were identical. What should be the difference between them, @cecton ?

[cecton]

It's been years for me haha 😅

Looking at the function name I guess the 2 tests are identical except for how they pass the file argument to the destream class. Probably at some point one was using a filename instead of a file object. It has to work for both (if that still exists in the API).

But don't take my word for it, try to use git blame on the file and see if they ever were different.

[jruere]

Why convert to pytest at all?

[eumiro]

The syntax with assert instead of self.assert* is nicer and more intuitive, the fixtures and parametrization makes the code more readable.

And it looks like touching the whole test codebase could show up some issues like this one :-)

[cecton]

I do prefer py.test too! I'm not sure it even existed when this project started.

Overall it's a nice improvement as it is easier to get in the code.

[eumiro]

And in the meantime it even dropped the dot from py.test to pytest. Okay, let me see what we can do. :-)

[eumiro]

But first, #22 needs to be reviewed/approved/merged.

[jruere]

Fair enough!

[eumiro]

Why didn't this get closed with #27?

[cecton]

It's because you need to write "Closes #21 " or "Fixes #21" in the commit message

[eumiro]

Does it have to be a commit message? I remember closing issues with PR messages like this one from our PR that I expected to close this issue:
#27 (comment)

[cecton]

Pretty sure (80%) it has to be in the commit message or the title of the commit (which is actually part of the message).

Some words also don't close the linked issue. For instance: "Related #21" will link but not close when the PR gets merged.

[eumiro]

Found it: in the PR number 19 I wrote Closes and the number 17 into the PR message. It even highlighted the word Closes. So maybe this has to be in the initial message (description) of a PR and not in a comment. 🤷