Port `unittest` classes to `pytest`. about pyoracc

[rillian]

Ok, great. It does seem like we don't really need separate parsers for each test, but it's better hygene to do so in case they develop more internal state at some point.

It looks like none of the tests calling compare_tokens reuses the lexer later, so perhaps the creation of a fresh lexer object can just be moved into to that function.

FWIW, porting test_atflex.py and test_atf_serializer.py lets tests pass on travis Python 3.7.

[ageorgou]

Thanks for raising this. I'm not sure what led to mixing both frameworks, but replacing unittest in test_atflex.py does seem straightforward.

I'm not sure this is the only reason for the failure with specific Python/pytest versions, but at least it explains why unittest is involved! Removing it should simplify things.

[rillian]

I see you assigned this to yourself. If it's helpful, I did some work on it.

I didn't parametrize compare tokens. It would remove passing through the fixture (maybe there's a more clever way to do that?) but we'd lose the specificity of the function names, and the comparison entries are hard enough to read one line at a time. Maybe you have better ideas?

[ageorgou]

By all means, feel free to contribute!

I think leaving compare_tokens as a separate helper function is reasonable - as you say, it would become too hard to read if parametrised. test_note_ended_by_line is shorter and looks fine the way you've done it.

I think you can avoid passing the lexer fixture explicitly in each test function with the autouse option. I also wonder whether we need one lexer per test, or whether it can be a module-scoped fixture (or class-scoped, if keeping all the tests in a class as before).

[ageorgou]

After #84, I think this only leaves test_atfparser.py to be ported.

The simplest way I see is to turn the lexer into a pytest-style fixture. The tests could also move at module-level rather than a class, but that would entail more changes and isn't necessary.
This patch seems to work:

diff --git a/pyoracc/test/atf/test_atfparser.py b/pyoracc/test/atf/test_atfparser.py
@@ -19,7 +19,6 @@ along with PyORACC. If not, see <http://www.gnu.org/licenses/>.
 '''
 
 

-from unittest import TestCase
 import pytest
 
 from pyoracc.atf.common.atflex import AtfLexer
@@ -39,9 +38,10 @@ from ...model.text import Text
 from ...model.translation import Translation
 
 
-class TestParser(TestCase):
+class TestParser:
     """A class that contains all tests of the ATFParser"""
-    def setUp(self):
+    @pytest.fixture(autouse=True)
+    def lexer(self):
         self.lexer = AtfLexer().lexer

The test functions only use the lexer in the try_parse method, so it could also just be created there instead of having a fixture (as done in #84). However, I like the separation in case the setup needs to become more complex in the future (especially as this is about testing the parsing rather than the lexer itself, compared to the changes in #84) - as unlikely as that seems at the moment.

Any thoughts, @rillian?

[rillian]

That's certainly the minimal change. You could even leave the method name the same!

If you don't have immediate plans for tests which need to access the lexer or parser state before or after calling try_parse, I would move it into the function where it's used. That puts all the initialization in one place, and allows removing both the repetition of self throughout the tests and the indent from class wrapper. It's not just less code to read; it makes clear there's no shared state to anyone reading the tests, and thus less for them to keep track of mentally.

In my experience, readability now is more important that ease of addition later, so this is the stronger argument. The github review interface is pretty good about marking indent-only changes, so making git blame harder to traverse is really the only downside of the larger change. But every project is different, and without the value of dropping the class wrapper I don't have a strong feeling about keeping the fixture.

If you do remove the class wrapper, I'd commit your minimal change first so there's an easier point to revert to if you need shared state later.

[ageorgou]

Closing this after #85, but feel free to reopen if you think there is something else left.

[rillian]

LGTM :)