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

MarkDecorator __eq__ override can lead to bad side effects #2758

Closed
astraw38 opened this issue Sep 7, 2017 · 3 comments · Fixed by #2764
Closed

MarkDecorator __eq__ override can lead to bad side effects #2758

astraw38 opened this issue Sep 7, 2017 · 3 comments · Fixed by #2764
Labels
good first issue easy issue that is friendly to new contributor type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously

Comments

@astraw38
Copy link

astraw38 commented Sep 7, 2017

The MarkDecorator __eq__ method assumes that it will always be compared with another mark. If you do large parametrizations based on lists, then you'll likely run into scenarios where this isn't the case (especially if you do combinations of lists).

Simple example:

import pytest
TEST_DATA = ['a', 'b', 'c', pytest.mark.xfail('d')]
SUBSET_TESTS = [x for x in TEST_DATA if x in ('a', 'b')]

@pytest.mark.parametrize('data', SUBSET_TESTS)
def test_me(data):
     pass

You'll get an error on collection:

    return self.mark == other.mark
E   AttributeError: 'str' object has no attribute 'mark'

This affects sets, lists (including list.index!), anything that checks for equality.

@RonnyPfannschmidt RonnyPfannschmidt added type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously good first issue easy issue that is friendly to new contributor labels Sep 7, 2017
@ghost
Copy link

ghost commented Sep 8, 2017

@astraw38 This seems to be easy to fix. Do you plan to create a PR on this? If not, can you assign this to me, @RonnyPfannschmidt ?

@RonnyPfannschmidt
Copy link
Member

@Xuanluong please go ahead and accompany it with a small unittest

@astraw38
Copy link
Author

astraw38 commented Sep 8, 2017

@Xuanluong I hadn't planned on it -- not for lack of desire, but lack of time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor 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