Converting tests to pytests discovers duplicated test code about destream HOT 12 CLOSED

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

from destream.

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.

from destream.

Why convert to pytest at all?

from destream.

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

from destream.

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.

from destream.

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

from destream.

Fair enough!

from destream.

Why didn't this get closed with #27?

from destream.

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

from destream.

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)

from destream.

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.

from destream.

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. 🤷

from destream.