Skip to content

Conversation

@SaturnFromTitan
Copy link
Contributor

@SaturnFromTitan SaturnFromTitan commented Feb 27, 2020

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd WillAyd added the Testing pandas testing functions or related to the test suite label Feb 27, 2020
@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Feb 27, 2020
@simonjayhawkins
Copy link
Member

Thanks @SaturnFromTitan

In order to close the issue can you do test_union_noncomparable in pandas\tests\indexes\ranges\test_setops.py as well.

There are several cases of @pytest.mark.parametrize("sort", [True, False]) as well. so having this fixture in top-level conftest maybe confusing.

for instance groupby where the default is True, unlike intersection where the default is False.

so is the None value in the fixture effectively True.

would tests break if the fixture was params=[None, True, False]?

@SaturnFromTitan
Copy link
Contributor Author

SaturnFromTitan commented Feb 27, 2020

Yes, the tests break when True is added to the fixture params:

ValueError: The 'sort' keyword only takes the values of None or False; True was passed.

I'll move the fixture to pandas/tests/indexes/conftest.py instead and adjust the docstring

@SaturnFromTitan
Copy link
Contributor Author

@simonjayhawkins I updated the docstring again, please have another look

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SaturnFromTitan changes lgtm.

@simonjayhawkins simonjayhawkins merged commit bf613c1 into pandas-dev:master Feb 28, 2020
roberthdevries pushed a commit to roberthdevries/pandas that referenced this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing pandas testing functions or related to the test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use sort fixture in more places

3 participants