Skip to content
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

Pytest 8.1.0 AttributeError: 'NoneType' object has no attribute 'setup_method' with staticmethod test #12065

Closed
anilbey opened this issue Mar 4, 2024 · 6 comments · Fixed by #12096
Labels
python pull requests that update Python code type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously

Comments

@anilbey
Copy link

anilbey commented Mar 4, 2024

Hi, the code that was working fine with pytest==8.0.2 started giving the error below for pytest==8.1.0.

Logs with pytest 8.0.2: https://github.com/BlueBrain/BlueCelluLab/actions/runs/8114860416/job/22181464420
Logs with pytest 8.1.0: https://github.com/BlueBrain/BlueCelluLab/actions/runs/8138274377/job/22239015810?pr=142

  request = <SubRequest '_xunit_setup_method_fixture_TestSonataSimulationAccess' for <Function test_init_file_not_found>>
  
      def xunit_setup_method_fixture(request) -> Generator[None, None, None]:
          instance = request.instance
          method = request.function
          if setup_method is not None:
  >           func = getattr(instance, setup_name)
  E           AttributeError: 'NoneType' object has no attribute 'setup_method'
  
  lib/python3.11/site-packages/_pytest/python.py:844: AttributeError

All of the installed dependencies and the system versions are available in the CI logs.

@bluetech
Copy link
Member

bluetech commented Mar 4, 2024

I see the failing test is a staticmethod:

https://github.com/BlueBrain/BlueCelluLab/blob/b8330e24c65219f48f0689cdc2d31b579586fb12/tests/test_circuit/test_simulation_access.py#L49-L50

I'm not sure if that's supposed to work (will check later), but curious if there's a particular reason you made it staticmethod?

@anilbey
Copy link
Author

anilbey commented Mar 4, 2024

Oh, thanks. No, in fact that does not have to be a staticmethod. Let me remove that and test again.

@anilbey
Copy link
Author

anilbey commented Mar 4, 2024

There exists another CI workflow (named "Examples") that is also passing on 8.0.2 and failing with 8.1.0. That does not use staticmethod.

8.0.2 Examples CI: https://github.com/BlueBrain/BlueCelluLab/actions/runs/8114860416/job/22181463086
8.1.0 Examples CI: https://github.com/BlueBrain/BlueCelluLab/actions/runs/8138274377/job/22239014458

@bluetech
Copy link
Member

bluetech commented Mar 4, 2024

@anilbey This is a different issue, the nbmake plugin will need to update its pytest_collect_file hookimpl to be compatible with pytest 8.1.

@anilbey
Copy link
Author

anilbey commented Mar 4, 2024

Update: removing 'staticmethod' worked.

Great, thanks a lot @bluetech for your quick reply in clarifying the issues.

@bluetech bluetech changed the title Pytest 8.1.0 AttributeError: 'NoneType' object has no attribute 'setup_method' Pytest 8.1.0 AttributeError: 'NoneType' object has no attribute 'setup_method' with staticmethod test Mar 4, 2024
@bluetech bluetech added type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously python pull requests that update Python code labels Mar 4, 2024
@bluetech
Copy link
Member

bluetech commented Mar 6, 2024

Bisected to c8792bd (pr #11780).

We're talking about this reproduction:

class TestIt:
    def setup_method(self): pass

    @staticmethod
    def test_1(): pass

This brings up the question, what is self in the setup_method of a staticmethod (or classmethod)?

The expectation, as happens with e.g. unittest and regular methods, is that it's a fresh instance.

However, pytest<=8.0 has a subtle bug: all of the static/classmethods use a shared instance. The two prints here are the same:

class TestIt:
    def setup_method(self): print('\nSETUP!', self)

    @staticmethod
    def test_1(): pass

    @classmethod
    def test_2(cls): pass

I think that fixing this preexisting bug will also fix the 8.1.0 crash, so I will look into it, though it probably won't be a simple fix, so I might instead restore the previous (problematic) behavior.

bluetech added a commit to bluetech/pytest that referenced this issue Mar 9, 2024
TODO: Clear `_instance` when `_obj` is cleared.

and also fixes a regression in pytest 8.0.0 where `setup_method` crashes
if the class has static or class method tests.

It is allowed to have a test class with static/class methods which
request non-static/class method fixtures (including `setup_method`
xunit-fixture). I take it as a given that we need to support this
somewhat odd scenario (stdlib unittest also supports it).

This raises a question -- when a staticmethod test requests a bound
fixture, what is that fixture's `self`?

stdlib unittest says - a fresh instance for the test.

Previously, pytest said - some instance that is shared by all
static/class methods. This is definitely broken since it breaks test
isolation.

Change pytest to behave like stdlib unittest here.

In practice, this means stopping to rely on `self.obj.__self__` to get
to the instance from the test function's binding. This doesn't work
because staticmethods are not bound to anything.

Instead, keep the instance explicitly and use that.

BTW, I think this will allow us to change `Class`'s fixture collection
(`parsefactories`) to happen on the class itself instead of a class
instance, allowing us to avoid one class instantiation. But needs more
work.

Fixes pytest-dev#12065.
bluetech added a commit to bluetech/pytest that referenced this issue Mar 9, 2024
and also fixes a regression in pytest 8.0.0 where `setup_method` crashes
if the class has static or class method tests.

It is allowed to have a test class with static/class methods which
request non-static/class method fixtures (including `setup_method`
xunit-fixture). I take it as a given that we need to support this
somewhat odd scenario (stdlib unittest also supports it).

This raises a question -- when a staticmethod test requests a bound
fixture, what is that fixture's `self`?

stdlib unittest says - a fresh instance for the test.

Previously, pytest said - some instance that is shared by all
static/class methods. This is definitely broken since it breaks test
isolation.

Change pytest to behave like stdlib unittest here.

In practice, this means stopping to rely on `self.obj.__self__` to get
to the instance from the test function's binding. This doesn't work
because staticmethods are not bound to anything.

Instead, keep the instance explicitly and use that.

BTW, I think this will allow us to change `Class`'s fixture collection
(`parsefactories`) to happen on the class itself instead of a class
instance, allowing us to avoid one class instantiation. But needs more
work.

Fixes pytest-dev#12065.
flying-sheep pushed a commit to flying-sheep/pytest that referenced this issue Apr 9, 2024
and also fixes a regression in pytest 8.0.0 where `setup_method` crashes
if the class has static or class method tests.

It is allowed to have a test class with static/class methods which
request non-static/class method fixtures (including `setup_method`
xunit-fixture). I take it as a given that we need to support this
somewhat odd scenario (stdlib unittest also supports it).

This raises a question -- when a staticmethod test requests a bound
fixture, what is that fixture's `self`?

stdlib unittest says - a fresh instance for the test.

Previously, pytest said - some instance that is shared by all
static/class methods. This is definitely broken since it breaks test
isolation.

Change pytest to behave like stdlib unittest here.

In practice, this means stopping to rely on `self.obj.__self__` to get
to the instance from the test function's binding. This doesn't work
because staticmethods are not bound to anything.

Instead, keep the instance explicitly and use that.

BTW, I think this will allow us to change `Class`'s fixture collection
(`parsefactories`) to happen on the class itself instead of a class
instance, allowing us to avoid one class instantiation. But needs more
work.

Fixes pytest-dev#12065.
nijel added a commit to nijel/translate that referenced this issue Apr 16, 2024
nijel added a commit to nijel/translate that referenced this issue Apr 16, 2024
nijel added a commit to translate/translate that referenced this issue Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python pull requests that update Python code type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants