[pytest-dev] fixtures and pylint W0621

[Brianna Laugher]

Hi,

Defining a fixture in the same file as a test that uses it triggers a
pylint complaint:

"W0621:*Redefining name %r from outer scope (line %s)* Used when a
variable?s name hide a name defined in the outer scope."

If the fixture was in conftest.py it would be fine. But it is nice to have
fixtures in the same file if they are only used there. Does everybody just
disable this message? I find it useful in general, I would prefer not to
disable it completely.

thanks
Brianna

[holger krekel]

Hi Brianna,

Post by Brianna Laugher
Hi,
Defining a fixture in the same file as a test that uses it triggers a
"W0621:*Redefining name %r from outer scope (line %s)* Used when a
variable?s name hide a name defined in the outer scope."
If the fixture was in conftest.py it would be fine. But it is nice to have
fixtures in the same file if they are only used there. Does everybody just
disable this message? I find it useful in general, I would prefer not to
disable it completely.

I use flakes and pep8 (pytest-flakes and pytest-pep8) and don't get this
error. What you can immediately do but which is not pretty is to use
the old naming scheme together with a decorator:

import pytest

@pytest.fixture
def pytest_funcarg__somefixture(...):
...

This should avoid the pylint warning.

Alternatively, putting the fixture on a class-level might
avoid the problem:

class TestClass:
@pytest.fixture
def fix(self):
....

def test_fix(self, fix):
...

but i haven't tried if pylint likes that.

best,
holger

[Brianna Laugher]

Hi,

Post by holger krekel
I use flakes and pep8 (pytest-flakes and pytest-pep8) and don't get this
error. What you can immediately do but which is not pretty is to use
import pytest
@pytest.fixture
...
This should avoid the pylint warning.

Ah, that's cool - I didn't realise that was possible.

However, it doesn't work the way you describe. :) Not sure if your
description or the code is backwards!

At the moment (pytest-2.3.5) if I have two fixtures like this:


@py.test.fixture
def pytest_funcarg__foo2(monkeypatch):
return monkeypatch


def pytest_funcarg__foo3(monkeypatch):
return monkeypatch


foo3 works (!). foo2 causes an assertion error on collection.

The code in parsefactories has a couple of if blocks, maybe one is inverted. :)

Don't know if this is a good idea, but you could tell if it was new
style/old style based on if the parameter was 'request' or
nothing/other fixtures.

cheers
Brianna

[holger krekel]

Post by Brianna Laugher

Post by holger krekel
I use flakes and pep8 (pytest-flakes and pytest-pep8) and don't get this
error. What you can immediately do but which is not pretty is to use
import pytest
@pytest.fixture
...
This should avoid the pylint warning.

Ah, that's cool - I didn't realise that was possible.
However, it doesn't work the way you describe. :) Not sure if your
description or the code is backwards!
@py.test.fixture
return monkeypatch
return monkeypatch
foo3 works (!). foo2 causes an assertion error on collection.
The code in parsefactories has a couple of if blocks, maybe one is inverted. :)

Right, it seems that when we introduced @pytest.fixture we decided you can
either use the prefix or the marker. That could be lifted but i wonder
if we should rather go for a different convention because pytest_funcarg__
is not a beautiful prefix. What do you think of pytest stripping the "__" prefix?

@pytest.fixture
def __foo2(monkeypatch):
return monkeypatch

This fixture would become accessible via the "foo2" name. Using "__foo2"
would yield a lookup error and the error would indicate there is a "foo2"
available. If you don't like it, any other suggestions?

Post by Brianna Laugher
Don't know if this is a good idea, but you could tell if it was new
style/old style based on if the parameter was 'request' or
nothing/other fixtures.

There is no difference between old-style and @pytest.fixture(scope="function")
and the presence of "request" indicates nothing particular.

best,
holger

Post by Brianna Laugher
cheers
Brianna
--
http://modernthings.org/

[Brianna Laugher]

Post by holger krekel
either use the prefix or the marker. That could be lifted but i wonder
if we should rather go for a different convention because pytest_funcarg__
is not a beautiful prefix. What do you think of pytest stripping the "__" prefix?
@pytest.fixture
return monkeypatch
This fixture would become accessible via the "foo2" name. Using "__foo2"
would yield a lookup error and the error would indicate there is a "foo2"
available. If you don't like it, any other suggestions?

Hmm. My main concern here would be that fixture definitions are easily
'findable' in a global search. The fixture decorator is easy to search
for, as is 'pytest_funcarg__'. Such an ugly prefix is also far easier
to google. Double underscore, not so much. The ugly prefix is also
good for backwards compat.

So, I don't really mind what happens as long as each fixture has at
least one of fixture decorator/ugly prefix.

cheers
Brianna

[holger krekel]

Post by Brianna Laugher

Post by holger krekel
either use the prefix or the marker. That could be lifted but i wonder
if we should rather go for a different convention because pytest_funcarg__
is not a beautiful prefix. What do you think of pytest stripping the "__" prefix?
@pytest.fixture
return monkeypatch
This fixture would become accessible via the "foo2" name. Using "__foo2"
would yield a lookup error and the error would indicate there is a "foo2"
available. If you don't like it, any other suggestions?

Hmm. My main concern here would be that fixture definitions are easily
'findable' in a global search. The fixture decorator is easy to search
for, as is 'pytest_funcarg__'. Such an ugly prefix is also far easier
to google. Double underscore, not so much. The ugly prefix is also
good for backwards compat.
So, I don't really mind what happens as long as each fixture has at
least one of fixture decorator/ugly prefix.

Well, ok. Pending further input, i made pytest accept pytest.fixture
decorated pytest_funcarg__ prefixed functions, see
https://bitbucket.org/hpk42/pytest/commits/aa1f0505a3156b9feca43cd67c5afc95622b9ac5

I am a bit unhappy because apart from the pylint warning it's also a
real issue for beginners to write something like this::

@pytest.fixture
def somename():
return 42

def test_something():
assert somename == 42

The "somename" is accessible (as a global) but is a function object.
The resulting error message is irritating if you are not proficient in python.
To prevent this irritation we would need to recommend a different
default way of declaring the fixture and neither pytest_funcarg__NAME nor
__NAME sound like we would like to suggest this as the main way in the docs.

Then again, my general recommendation is to put fixture functions into
conftest.py files and you then get a clean "NameError" in a test module
for the above example. The docs use the "everything contained in test module"
method, though, to make it easier to present getting-started examples.
Maybe it makes sense to mention this recommended conftest/test module
split earlier on.

best,
holger

[lahwran]

perhaps pytest.fixture could return a special object that rejects all
operations, rather than the decorated function object; such that `theobject
== 5` would produce a big flashy error in theobject.__eq__ indicating you
need to actually use the fixture for this to work, maybe even with an
example. Repeat for all magic methods that make sense. this object would
maybe have a _pytest_fixture attribute, or conform to whatever protocol
already exists for detecting fixture objects, such that the only thing you
could do with such an object is check if it's a fixture and get the
original function object from somewhere inside it. Any other operation
would result in a flashy error.

Post by holger krekel
can

Post by Brianna Laugher

Post by holger krekel
either use the prefix or the marker. That could be lifted but i wonder
if we should rather go for a different convention because

pytest_funcarg__

Post by Brianna Laugher

Post by holger krekel
is not a beautiful prefix. What do you think of pytest stripping the

"__" prefix?

Post by Brianna Laugher

Post by holger krekel
@pytest.fixture
return monkeypatch
This fixture would become accessible via the "foo2" name. Using

"__foo2"

Post by Brianna Laugher

Post by holger krekel
would yield a lookup error and the error would indicate there is a

"foo2"

Post by Brianna Laugher

Post by holger krekel
available. If you don't like it, any other suggestions?

Hmm. My main concern here would be that fixture definitions are easily
'findable' in a global search. The fixture decorator is easy to search
for, as is 'pytest_funcarg__'. Such an ugly prefix is also far easier
to google. Double underscore, not so much. The ugly prefix is also
good for backwards compat.
So, I don't really mind what happens as long as each fixture has at
least one of fixture decorator/ugly prefix.

Well, ok. Pending further input, i made pytest accept pytest.fixture
decorated pytest_funcarg__ prefixed functions, see
https://bitbucket.org/hpk42/pytest/commits/aa1f0505a3156b9feca43cd67c5afc95622b9ac5
I am a bit unhappy because apart from the pylint warning it's also a
@pytest.fixture
return 42
assert somename == 42
The "somename" is accessible (as a global) but is a function object.
The resulting error message is irritating if you are not proficient in python.
To prevent this irritation we would need to recommend a different
default way of declaring the fixture and neither pytest_funcarg__NAME nor
__NAME sound like we would like to suggest this as the main way in the docs.
Then again, my general recommendation is to put fixture functions into
conftest.py files and you then get a clean "NameError" in a test module
for the above example. The docs use the "everything contained in test module"
method, though, to make it easier to present getting-started examples.
Maybe it makes sense to mention this recommended conftest/test module
split earlier on.
best,
holger
_______________________________________________
Pytest-dev mailing list
Pytest-dev at python.org
https://mail.python.org/mailman/listinfo/pytest-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/pytest-dev/attachments/20131204/05617af7/attachment.html>

[Floris Bruynooghe]

Hi,

Post by lahwran
perhaps pytest.fixture could return a special object that rejects all
operations, rather than the decorated function object; such that `theobject
== 5` would produce a big flashy error in theobject.__eq__ indicating you
need to actually use the fixture for this to work, maybe even with an
example. Repeat for all magic methods that make sense. this object would
maybe have a _pytest_fixture attribute, or conform to whatever protocol
already exists for detecting fixture objects, such that the only thing you
could do with such an object is check if it's a fixture and get the original
function object from somewhere inside it. Any other operation would result
in a flashy error.

This seems like a quite nice idea to me.

Post by lahwran

Post by holger krekel
Well, ok. Pending further input, i made pytest accept pytest.fixture
decorated pytest_funcarg__ prefixed functions, see
https://bitbucket.org/hpk42/pytest/commits/aa1f0505a3156b9feca43cd67c5afc95622b9ac5

I don't really like this change. IIRC this was considered when the
decorator was introduced and the reason it was not allowed originally
is because the signature is different between the different ways of
defining fixtures. That can be very confusing too.

My first reaction on this pylint issue was to see if this can not be
fixed wit a pylint plugin. And I still think this would be nicer then
the other proposals of trying to figure out how to name fixtures
differently. The way of declaring fixtures right now is very nice and
readable. Especially if lahwran's suggestion is implemented as that
would make any mistakes obvious (and to an experience python developer
they already are obvious).

Post by lahwran

Post by holger krekel
Then again, my general recommendation is to put fixture functions into
conftest.py files and you then get a clean "NameError" in a test module
for the above example. The docs use the "everything contained in test module"
method, though, to make it easier to present getting-started examples.
Maybe it makes sense to mention this recommended conftest/test module
split earlier on.

Personally I tend to recommend people to declare their fixtures as
close to the code using it. So if a fixture is only used in one class
declare it in that class. I find it easier to manage which fixtures
are needed and know about the relevant ones without having to search
around. Also if you have all test modules in package, like py.test,
you quickly end up with colliding fixture names and a giant and hard
to manage collection of fixtures in conftest.py.


Regards,
Floris

[holger krekel]

Hi Floris, all,

Post by Floris Bruynooghe

Post by lahwran
perhaps pytest.fixture could return a special object that rejects all
operations, rather than the decorated function object; such that `theobject
== 5` would produce a big flashy error in theobject.__eq__ indicating you
need to actually use the fixture for this to work, maybe even with an
example. Repeat for all magic methods that make sense. this object would
maybe have a _pytest_fixture attribute, or conform to whatever protocol
already exists for detecting fixture objects, such that the only thing you
could do with such an object is check if it's a fixture and get the original
function object from somewhere inside it. Any other operation would result
in a flashy error.

This seems like a quite nice idea to me.

Not quite sure. It would mean that if you want to unittest
fixture functions, it's slightly hairy. More importantly,
i think sometimes i use (and probably others as well) direct
calling of fixture functions for other purposes already:

@pytest.fixture
def arg(request):
...

@pytest.fixture
def arg2(request):
if some_condition:
val = arg(request)
...

which is currently valid code. Arguably one could use getfuncargvalue
but IIRC i had some reasons to not do it at the time. (Can't locate the
code currently).

That being sad, i am open to trying it -- so if somebody does a PR
introducing the "exploding" @pytest.fixture return value, i'd merge
it temporarily and see if there are any problems. It would be very
nice if random errors of forgetting to specify an argument would
result in a clear error message.

Post by Floris Bruynooghe

Post by lahwran

Post by holger krekel
Well, ok. Pending further input, i made pytest accept pytest.fixture
decorated pytest_funcarg__ prefixed functions, see
https://bitbucket.org/hpk42/pytest/commits/aa1f0505a3156b9feca43cd67c5afc95622b9ac5

I don't really like this change. IIRC this was considered when the
decorator was introduced and the reason it was not allowed originally
is because the signature is different between the different ways of
defining fixtures. That can be very confusing too.

Not sure i follow. What is different? The only difference is that
the decorator allows to specify a caching scope, params etc. It does
not change anything about the fixture functions own signature.

Post by Floris Bruynooghe
My first reaction on this pylint issue was to see if this can not be
fixed wit a pylint plugin. And I still think this would be nicer then
the other proposals of trying to figure out how to name fixtures
differently. The way of declaring fixtures right now is very nice and
readable. Especially if lahwran's suggestion is implemented as that
would make any mistakes obvious (and to an experience python developer
they already are obvious).

Aiming for better pylint integration eg. through a plugin sounds like a
great plan. And one that does not require deep pytest knowledge -- so
if someone here on the list is interested, i am willing to help design it.

Post by Floris Bruynooghe

Post by lahwran

Post by holger krekel
Then again, my general recommendation is to put fixture functions into
conftest.py files and you then get a clean "NameError" in a test module
for the above example. The docs use the "everything contained in test module"
method, though, to make it easier to present getting-started examples.
Maybe it makes sense to mention this recommended conftest/test module
split earlier on.

Personally I tend to recommend people to declare their fixtures as
close to the code using it. So if a fixture is only used in one class
declare it in that class. I find it easier to manage which fixtures
are needed and know about the relevant ones without having to search
around. Also if you have all test modules in package, like py.test,
you quickly end up with colliding fixture names and a giant and hard
to manage collection of fixtures in conftest.py.

True as well. Although for my 1K-10K LOC projects it's not really an issue
yet. Maybe some day we can think about namespacing in some way.

best,
holger

[Floris Bruynooghe]

Post by holger krekel

Post by Floris Bruynooghe

Post by holger krekel
Well, ok. Pending further input, i made pytest accept pytest.fixture
decorated pytest_funcarg__ prefixed functions, see
https://bitbucket.org/hpk42/pytest/commits/aa1f0505a3156b9feca43cd67c5afc95622b9ac5

I don't really like this change. IIRC this was considered when the
decorator was introduced and the reason it was not allowed originally
is because the signature is different between the different ways of
defining fixtures. That can be very confusing too.

Not sure i follow. What is different? The only difference is that
the decorator allows to specify a caching scope, params etc. It does
not change anything about the fixture functions own signature.

I was assuming the old fixtures do not allow requesting other fixtures
via funcargs. But I might well be wrong on that.


Regards,
Floris

[holger krekel]

Post by Floris Bruynooghe

Post by holger krekel

Post by Floris Bruynooghe

Post by holger krekel
Well, ok. Pending further input, i made pytest accept pytest.fixture
decorated pytest_funcarg__ prefixed functions, see
https://bitbucket.org/hpk42/pytest/commits/aa1f0505a3156b9feca43cd67c5afc95622b9ac5

I don't really like this change. IIRC this was considered when the
decorator was introduced and the reason it was not allowed originally
is because the signature is different between the different ways of
defining fixtures. That can be very confusing too.

Not sure i follow. What is different? The only difference is that
the decorator allows to specify a caching scope, params etc. It does
not change anything about the fixture functions own signature.

I was assuming the old fixtures do not allow requesting other fixtures
via funcargs. But I might well be wrong on that.

Indeed, old pytest_funcarg__fixture already accepted other fixtures as
arguments, so no difference there.

However, i now think and agree that adding @pytest.fixture markers to
old-style pytest_funcarg__NAME fixtures is a bit backwards. Going for
a pylint plugin probably makes more sense. So i just backed out
the change (so now you cannot use @pytest.fixture on pytest_funcarg__NAME).

best,
holger

Post by Floris Bruynooghe
Regards,
Floris
--
Debian GNU/Linux -- The Power of Freedom
www.debian.org | www.gnu.org | www.kernel.org

[Brianna Laugher]

Hi,

Post by holger krekel

Post by Floris Bruynooghe
My first reaction on this pylint issue was to see if this can not be
fixed wit a pylint plugin. And I still think this would be nicer then
the other proposals of trying to figure out how to name fixtures
differently. The way of declaring fixtures right now is very nice and
readable. Especially if lahwran's suggestion is implemented as that
would make any mistakes obvious (and to an experience python developer
they already are obvious).

Aiming for better pylint integration eg. through a plugin sounds like a
great plan. And one that does not require deep pytest knowledge -- so
if someone here on the list is interested, i am willing to help design it.

I agree, I think a plugin is the way to go.

I have made a bit of an initial stab at a plugin:

https://bitbucket.org/pfctdayelise/pylint-pytest/

I am not sure the string building approach will get us very far, but I
just wanted to get something that would at least run (a lot of the
pylint docs are out of date).

Someone should probably run pylint over the pytest src to see what
other pylint messages are common.

cheers
Brianna

--
They've just been waiting in a mountain for the right moment:
http://modernthings.org/

[Floris Bruynooghe]

Hi Brianna,

Post by Brianna Laugher
I agree, I think a plugin is the way to go.
https://bitbucket.org/pfctdayelise/pylint-pytest/

I get a message saying I have no permission to access the repository
when following that URL.


Regards,
Floris

[Brianna Laugher]

Sorry, I accidentally made it private - should be accessible now.

Brianna

Post by holger krekel
Hi Brianna,

Post by Brianna Laugher
I agree, I think a plugin is the way to go.
https://bitbucket.org/pfctdayelise/pylint-pytest/

I get a message saying I have no permission to access the repository
when following that URL.
Regards,
Floris
--
Debian GNU/Linux -- The Power of Freedom
www.debian.org | www.gnu.org | www.kernel.org

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/pytest-dev/attachments/20131211/f12718c6/attachment.html>