"AttributeError: object has no attribute 'add_mark'" when marks clash with existing attributes

[DRMacIver]

When running the following code with python -m pytest foo.py:

import pytest


def assign_foo(f):
    f.foo = "Hi"
    return f


@pytest.mark.foo
@assign_foo
def test():
    pass

I get the following error:

foo.py:10: in <module>
    @assign_foo
v/lib/python3.6/site-packages/_pytest/mark/structures.py:213: in __call__
    store_legacy_markinfo(func, self.mark)
v/lib/python3.6/site-packages/_pytest/mark/structures.py:250: in store_legacy_markinfo
    holder.add_mark(mark)
E   AttributeError: 'str' object has no attribute 'add_mark'

This is on 3.6.0 with the following packages in the virtualenv (i.e. a clean install of pytest 3.6.2):

Package        Version
-------------- -------
atomicwrites   1.1.5
attrs          18.1.0
more-itertools 4.2.0
pip            10.0.1
pluggy         0.6.0
py             1.5.4
pytest         3.6.2
setuptools     39.2.0
six            1.11.0
wheel          0.31.1

We encountered this because someone was applying @pytest.mark.hypothesis to a @given decorated test (see HypothesisWorks/hypothesis#1362), which broke when we added a hypothesis attribute on the decorated function.

I believe this is only a problem with the legacy support and is not required for marks to work: we already apply a hypothesis mark to all @given decorated tests, and it is still working fine, so it seems likely that a fix that checked if the attribute was already present and did nothing if it was not a MarkInfo would be backwards compatible and fix this problem.

[pytestbot]

GitMate.io thinks possibly related issues are #49 (AttributeError: 'Session' object has no attribute '_setupstate'), #362 (AttributeError: 'MyOptionParser' object has no attribute 'parse_known_args'), #2162 (AttributeError: 'SubRequest' object has no attribute 'param'), #1310 (AttributeError: '_HookCaller' object has no attribute 'spec_opts'), and #1447 (AttributeError: 'OutputWriter' object has no attribute 'isatty').

[RonnyPfannschmidt]

good find and thanks for the minimal case, i believe introducing a warning there and doing nothing is indeed a good way forward there

@nicoddemus i'd appreciate your opinion

[DRMacIver]

introducing a warning there

What would the warning be? What should the user be doing differently in this case?

Given that the more modern mark storage mechanism means that (as far as I can tell) it's perfectly fine to have an attribute and a mark of the same name, it seems a shame to let the legacy support turn reasonable behaviour into a warning unless there's something the user can do that will avoid that.

(In the case of Hypothesis this doesn't matter because the fix for this problem was just to tell the use "Don't do that then" as we're already applying the mark, I'm just not a fan of non-actionable warnings)

[RonnyPfannschmidt]

@DRMacIver the warning is necessary since we dont act on the actual instruction of applying a mark anymore - aka we no longer add something the code says we do

[DRMacIver]

we dont act on the actual instruction of applying a mark anymore - aka we no longer add something the code says we do

I don't think that's true though, unless I'm misunderstanding the code.

The problem is occurring only in the part of the code that applies the legacy mark info, which adds an attribute to the function. The part that adds it to the pytestmark attribute should continue to work unchanged AFAICT and so the mark would continue to work properly, it just wouldn't be available via an attribute on the function.

[RonnyPfannschmidt]

@DRMacIver the legacy storage is the only storage pre pytest 3.6 and the api of it is expected to work - just because its deprecated, it should just start to work worse than it already is without telling users

[RonnyPfannschmidt]

i started to review history, the issue got introduced by me earlier, im now tracking back to the version that introduces it

[RonnyPfannschmidt]

i finished the evaluation, the call was always broken, on the feature branch we should opt to do nothing in the case, thanks for calling me out on the warning