Use features from Pytest and remove Unittest

[bryanwweber]

Abstract

Pytest has a bunch of nice features that have potential to make the Python test code shorter, easier to read, and potentially faster. Recent work has made it possible to use Pytest with the existing unittest-based suite, but I'm interested in exploring what we can get if we drop unittest altogether.

Motivation

Hoping to improve the test suite. Wanting to use Pytest features like fixtures to simplify the test code.

Description

I've been working on a branch over here: https://github.com/bryanwweber/cantera/tree/use-pytest, specifically, I've copied almost all the tests from the test_purefluid.py file into a new file (test_purefluid_pytest.py). The code is about 100 lines shorter, but it's missing a bunch of comments and such that we'd probably want to copy over, and the sample data has been copied out into YAML files. On the other hand, it doesn't require any of the utilities.CanteraTest infrastructure yet, so that would save some lines... So, so far, there hasn't been much benefit in terms of lines of code.

I haven't timed it yet either. Arguably, the code is easier to read, but it's really largely the same. I do like the new create_fluid fixture for the various PureFluid tests.

The main benefit I see so far is standardizing on np.isclose to test floating point numbers rather than our bespoke assertNear. math.isclose or pytest.approx would be more-or-less equivalent.

I'm putting this up as a work-in-progress issue because I've started work on it, but if there doesn't seem to be a real benefit, I'm fine with dropping it. I might like to try one more file, maybe a more complicated case, to see if there's any real benefit from fixtures over the CanteraTest class.

[ischoegl]

Hi @bryanwweber ... thanks for posting! I am overall very much in favor of migrating to pytest, as it is imho a better/more flexible testing environment compared to unittest.

One thing I noticed is that in your code sample you went away from using classes to group tests (a feature that I believe is powerful). While I don't have extensive experience with fixtures, one thing I have used before is

@pytest.fixture(scope="class")
def fixture1(request):
    # this is run once per class instantiation
    class_var = "some_class_variable"
    if request.cls is not None:
        request.cls._some_variable = class_var # this is accessible in tests
    yield class_var

@pytest.fixture(scope="class")
def fixture2(request):
    # [...]

@pytest.mark.usefixtures("fixture1", "fixture2")
class TestSomething:
    def test_this(self):
        assert abc
    def test_that(self):
        assert xyz

(one caveat is that what I used still had some remnants of unittest but I think what I sketched above should work). In other words, there may be solutions that could recycle some of the existing structure.

[bryanwweber]

In other words, there may be solutions that could recycle some of the existing structure.

Thanks for the comments @ischoegl! I wonder about this suggestion, and I hope you can elaborate if I give a little more context... It was a deliberate choice to move to module-level functions, unless the class provided some useful grouping features, like with the common comparison data. Since pytest doesn't require the class based structure, what advantages do you see of keeping it in this code?

[ischoegl]

@bryanwweber ... I was indeed wondering about the rationale, so this being intentional makes sense. As an aside, I eventually did notice that you used a class structure at the very end of your test anyways!

The main advantage I see in having classes is that it allows for grouping of tests that probe a specific topic (it is imho much easier to figure out what is being done). Also, if you look at some of the tests I wrote in test_reaction.py (and the upcoming test_jacobian.py from Cantera/cantera#1089), the tests are designed to systematically cover different variants of ReactionRate and Reaction specializations. I think this strategy would map the same to pytest, where fixtures would replace setUpClass or setUp tasks. One thing I don't have an answer for yet is that the unittest approach naturally lends itself to support these cascading tests (where specialized test classes can add additional tests, which is imho a nice feature). I'm sure that similar things can be done for pytest ... . But it's a different paradigm.

[speth]

I'd say overall, I think making PyTest a dependency and relying on some of its features is fine. I can see some benefit to using PyTest's "clever" introspection of standard assert statements and shifting away from both the assertSomething methods as well as the custom ones introduced in CanteraTest.

I'm less convinced that there's a lot to be gained by a wholesale rewrite of the existing test suite, though (a mere 13,000 lines...). You're already seeing that this doesn't make the code any shorter. I guess for my part, I don't really see why pytest.fixture (at least as used here) is significantly better than unittest.TestCase.setUp.

[bryanwweber]

I guess for my part, I don't really see why pytest.fixture (at least as used here) is significantly better than unittest.TestCase.setUp

Yeah, I definitely agree with this sentiment. I think there are some details about how and when the function/method is executed that can make a difference in some cases (perhaps more likely with tear-down methods), I can look into it some more!

[ischoegl]

fwiw, I think that pytest.fixtures are quite useful for repetitive tasks (e.g. loading and setting up Solution objects, potentially in conftest.py), but they are hard to customize. Once you customize, unittest.TestCase.setUp shines ... as pytest and unittest are compatible, mix-and-match may be the best of both worlds?

My take on this would be to go ahead and require pytest for 2.6, and remove other Python unittest support from SCons?

[bryanwweber]

but they are hard to customize.

@ischoegl I'm curious again about this statement 😊 this has been the opposite of my experience. Since any function or class can be a fixture, I find them much easier to customize. You're not restricted to OOP inheritance paradigm like with unittest.

[speth]

remove other Python unittest support from SCons?

What does this mean? Just dropping support for unittest as the test runner, given all the existing tests are built on unittest.TestCase?

[ischoegl]

but they are hard to customize.

I figure that this is not strictly true, as pytest customization options are limitless :joy:.

I guess it would be more accurate to say that unittest makes it easy to leverage OOP. I personally find OOP easy and straight-forward to understand, plus it automatically leads to some organization of code. On the pytest side, applying fixtures to individual test functions can make things quite hard to locate - some of the test files run well beyond 1000 lines.

[ischoegl]

What does this mean? Just dropping support for unittest as the test runner, given all the existing tests are built on unittest.TestCase?

Correct. Just remove the option for pytest = None here https://github.com/Cantera/cantera/blob/ec85c244347490cd52d534d79c8234d6a1c75ce2/test/python/runCythonTests.py#L29-L32 and anything below line 106.

[ischoegl]

@speth and @bryanwweber

See Cantera/cantera#1167. With this proposed update, any pytest feature would become usable. This doesn't really address the main gist of this issue as raised though, as unittest.TestCase remains untouched ...

[bryanwweber]

One potential concrete improvement by avoiding unittest-style inheritance is the ability to run tests in parallel. According to the Python docs, setUpClass and tearDownClass are called every time the test runner encounters a new class (I'm not sure if subclasses count?). This means that if tests are run in parallel, or in a different order than source order, the setup/teardown methods may be executed multiple times. I presume pytest operates similarly when using unittest-style code. However, I think the scope of the fixtures is quite explicit about when the function will be called, and teardown can be accomplished by yielding from the fixture instead of returning. Also, there is a built-in tempdir fixture, which is the only major thing we use setUpClass and tearDownClass for.

[ischoegl]

@bryanwweber ... yes, I do recall something about improved efficiency for pytest. Regardless, Cantera/cantera#1167 should be a (small) step in this direction. As @speth mentioned, porting 13000+ lines is a tough proposition, even if we lose part of that once XML/CTI is removed. But pytest would be interesting for new features like #114 (as long as the test suite is run from Python of course :joy:).

[bryanwweber]

@ischoegl Sure, there are libraries like hypothesis that can do that kind of property-based testing from Python. Pytest can also run Google C++ tests via https://github.com/pytest-dev/pytest-cpp although it doesn't compile the tests.

[ischoegl]

One middle ground here is to use pytests's classic x-unit style setup

I've been pretty successful converting tests to the format

    @classmethod
    def setup_class(cls):
        [...]

    @classmethod
    def teardown_class(cls):
        [...]

    def setup_method(self, method):
        [...]

in unrelated work.