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 silently chooses the wrong fixture when two plugins declare a fixture with the same name #3966

Open
burnpanck opened this issue Sep 11, 2018 · 12 comments
Labels
topic: fixtures anything involving fixtures directly or indirectly topic: reporting related to terminal output and user-facing messages and errors type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog

Comments

@burnpanck
Copy link

burnpanck commented Sep 11, 2018

The magic name-matching of pytest's fixtures can lead to namespace clashes, apart from being unpythonic (see #3834). Getting a wrong fixture may lead to hard-to-debug errors, especially if one does not know the plugin code and therefore does not know if one plugin may legitimately call the other plugin's code. Thus, I believe that pytest should not allow a fixture to be used whose name clashes between two declarations (Python Zen: In the face of ambiguity, refuse the temptation to guess.). Instead, it should raise an explicit error, including a hint to the test-writer how they may resolve the ambiguity.

The environment where this bug was observed was the following:

python 3.6.1
This is pytest version 3.5.0
  pytest-sanic-0.1.13
  pytest-benchmark-3.1.1
  pytest-aiohttp-0.3.0

where both aiohttp and sanic provide a test_client fixture.

@Zac-HD Zac-HD added topic: reporting related to terminal output and user-facing messages and errors type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog topic: fixtures anything involving fixtures directly or indirectly labels Oct 19, 2018
@Zac-HD
Copy link
Member

Zac-HD commented Oct 19, 2018

I support pytest raising an explicit error whenever it would otherwise choose between multiple fixtures 👍

IMO this has never worked and we could go to an error straight away, but if others disagree we might need a deprecation period first.

@RonnyPfannschmidt
Copy link
Member

we have to account for layered fixture overrides however

def myfixture(myfixture): ... is supported as a customization system for plugins

additionally i believe pytest-selenium uses those overrides in order to allow local testsuites to customize certain settings

@Zac-HD
Copy link
Member

Zac-HD commented Oct 19, 2018

😱... this isn't a fatal problem though, because in this case it's always the more-recently-defined fixture that should be chosen.

We can therefore ignore "parent" fixtures, but still error on unrelated or "sibling" fixtures.

@RonnyPfannschmidt
Copy link
Member

detecting sieblings correctly will be tricky

i propose to introduce a flag for fixtures that are supposed to be overridden, and then only warning on fixtures that override fixtures that are not meant for overriding

@RonnyPfannschmidt
Copy link
Member

that way we can count the existence of myfixture(myfixture) as externally allowed override and and a fixture(..., allow_override=True) marking as internally allowed override

@Zac-HD
Copy link
Member

Zac-HD commented Oct 19, 2018

detecting siblings correctly will be tricky

So long as we can tell what (if any) fixtures a given fixture layers on, it's just a matter of walking back up that tree looking for the conflicting fixtures. And if there are no conflicting fixtures we don't need to do anything, so the performance should be fine.

Creating a distinction between fixtures that can and cannot be overridden seems likely to cause lots of subtle problems about ordering and sequences of fixtures, so I'd avoid it.

@RonnyPfannschmidt
Copy link
Member

we already have a situation where overriding is used as a feature, so we cant break it
we have explicit overrides where we take in the original fixture as a input value, and we have implicit overrides, where a different definition is supposed to be allowed to be overridden

from my pov we want to to warn of all overrides but

  • we want to warn which fixture was overridden,
  • we want to ignore an override if the override takes the original fixture as an argument
  • we want to ignore the override if the original fixture declares that it is supposed to be overridden

@Zac-HD
Copy link
Member

Zac-HD commented Oct 19, 2018

Yep, that sounds like a good plan 🚀

@dhensen
Copy link

dhensen commented Dec 10, 2021

Is there some kind of summary of what the current status is? I'm finding this in 2021 after weird errors using pandas. It turns out pandas defines a fixture called s3_resource. We similarly named fixture in our own code. I think we will run into more problems with generically named fixtures (think of this: s3_client, ddb_client, ddb_resource, etc....).

Ideally I do not want to rename. Currently I don't see any warning about duplicate fixtures, at a certain point I started getting failing tests. It seems random, I don't get it yet. Using pytest 6.2.2

@dhensen
Copy link

dhensen commented Dec 10, 2021

Excuse me, turned out I made a mistake. When creating a bucket with s3 resource (instead of client) you have to specify a CreateBucketConfiguration. I did not do that.

I could conclude I was wrong after having read the docs about fixture scopes and how pytest resolves fixtures when third party libs are involved.

@crashvb
Copy link

crashvb commented Feb 1, 2022

Is there a workaround, or way to select between the two conflicting fixtures? If so, does it apply to nested fixture resolution?

I am experiencing this issue after import two libraries that provide the same fixtures of the same name, as-well-as, those fixtures using nested fixtures of the same name ...

crashvb added a commit to crashvb/pytest-docker-registry-fixtures that referenced this issue Feb 1, 2022
crashvb added a commit to crashvb/pytest-docker-haproxy-fixtures that referenced this issue Feb 1, 2022
@nicoddemus
Copy link
Member

There's no workaround currently AFAIK, I'm afraid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: fixtures anything involving fixtures directly or indirectly topic: reporting related to terminal output and user-facing messages and errors type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog
Projects
None yet
Development

No branches or pull requests

6 participants