Skip to content

TST: Use sort fixture in more places #32292

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

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