-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use features from Pytest and remove Unittest #128
Comments
Hi @bryanwweber ... thanks for posting! I am overall very much in favor of migrating to 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 |
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? |
@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 |
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 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 |
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! |
fwiw, I think that My take on this would be to go ahead and require |
@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 |
What does this mean? Just dropping support for |
I figure that this is not strictly true, as I guess it would be more accurate to say that |
Correct. Just remove the option for |
@speth and @bryanwweber See Cantera/cantera#1167. With this proposed update, any |
One potential concrete improvement by avoiding |
@bryanwweber ... yes, I do recall something about improved efficiency for |
@ischoegl Sure, there are libraries like |
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. |
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 theutilities.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 bespokeassertNear
.math.isclose
orpytest.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.The text was updated successfully, but these errors were encountered: