Skip to content

Docs: Add details on possible leakage when using test classes #8252

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

Closed
christian-steinmeyer opened this issue Jan 18, 2021 · 3 comments · Fixed by #8276
Closed

Docs: Add details on possible leakage when using test classes #8252

christian-steinmeyer opened this issue Jan 18, 2021 · 3 comments · Fixed by #8276
Labels
type: docs documentation improvement, missing or needing clarification

Comments

@christian-steinmeyer
Copy link
Contributor

Hi! I just ran into a similar problem to this one, where the result of tests depended on the order in which they were run. The root cause of this was that I defined attributes on the TestClass's level and mocked methods of these attributes. Eventually I found out that this could be overcome by using the setup_method of the class instead of class attributes. When going over the docs for grouping multiple tests in a class, I found no hint of this possible problem. Others might run into this problem as well and there already exists proper documentation of the solution here.

May I suggest to replace the following sentences by a more accurate documentation? As there seem to be cases in which it is not 100% true, I find it possibly misleading (e.g. when combining pytest with mocking).

Something to be aware of when grouping tests inside classes is that each test has a unique instance of the class. Having each test share the same class instance would be very detrimental to test isolation and would promote poor test practices.

@nicoddemus
Copy link
Member

Hi @christian-steinmeyer,

Thanks for the suggestion!

May I suggest to replace the following sentences by a more accurate documentation?

Not sure I understand, that sentence is accurate to me. What do you have in mind? Also, would you consider writing a PR with what you believe would be a better guidance? We can then discuss this over a more concrete suggestion. 👍

@Zac-HD Zac-HD added the type: docs documentation improvement, missing or needing clarification label Jan 19, 2021
@christian-steinmeyer
Copy link
Contributor Author

Hi again. I'll have to apologize. I believe I confused class attributes with instance attributes, but should have known better. To elaborate, given a class like this:

class TestMyClass:
    my_attribute = SomeObject()

    def setup_method(self):
        self.my_other_attribute = SomeObject()

    def test_a(self):
        self.my_attribute.increment()
        self.my_other_attribute.increment()
        assert self.my_attribute() == self.my_other_attribute()

    def test_b(self):
         assert self.my_attribute() == self.my_other_attribute()

under the assumption that SomeObject is a simple class that is comparable and only contains a value which can be incremented by increment(), I had assumed, this should work. Now I realized that it only works if running test_b before test_a, because test_a changes a class attribute and that is maintained throughout the test run (i.e., "leaks" from the test). But I believe, this is desired behavior (e.g. for connections).

As a result, you are 100% correct, the original documentation is accurate. Nevertheless one might add another hint for people like me. Perhaps something along the lines of:

Something to be aware of when grouping tests inside classes is that each test has a unique instance of the class. Having each test share the same class instance would be very detrimental to test isolation and would promote poor test practices. Note, that attributes added at class level (e.g. outside of the setup_method(self)), will be shared between tests.

@nicoddemus
Copy link
Member

Hi again. I'll have to apologize. I believe I confused class attributes with instance attributes, but should have known better.

No worries.

Nevertheless one might add another hint for people like me.

I'm OK with adding that note, would you like to contribute a PR with that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs documentation improvement, missing or needing clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@nicoddemus @christian-steinmeyer @Zac-HD and others