Skip to content

CLN: Split test_window.py #27305

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

Merged
merged 5 commits into from
Jul 10, 2019
Merged

Conversation

mroeschke
Copy link
Member

@mroeschke mroeschke commented Jul 9, 2019

xref #19228, #26807

  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

test_window.py was getting fairly unwieldily. There a couple test classes that use a non-pytest-idomatic Base class, so those test classes are kept together for now. I split out the rest of the test classes into separate files as they made sense.

I think the classes in test_window.py need further untangling before a conftest.py file can be made for the new directory.

Open to additional quick win optimizations.

cc @jreback

@mroeschke mroeschke added Clean Testing pandas testing functions or related to the test suite Window rolling, ewma, expanding labels Jul 9, 2019
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.

Thanks - this module could definitely use a little refactor.

Not opposed to this move but thinking through if we should parametrize first instead

self.data = self._create_dtype_data(self.dtype)
self.expects = self.get_expects()

def test_dtypes(self):
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only function being tested in this module? If so I think would make sense to try and parametrize this first before moving


def test_ragged_std(self):

df = self.ragged
Copy link
Member

Choose a reason for hiding this comment

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

Good case for parametrization here (and a few functions below)

@@ -1,6 +1,6 @@
from collections import OrderedDict
from datetime import datetime, timedelta
from itertools import product
from typing import Any
Copy link
Member

Choose a reason for hiding this comment

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

Not opposed to this but note that we haven't done annotations in any other test module nor do we check it (mypy.ini excludes these) so would be OK to leave out for now as well



@pytest.fixture(params=[None, 1])
def min_periods(request) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

Optional[int] would be preferable as the return value here

@jreback
Copy link
Contributor

jreback commented Jul 9, 2019

@mroeschke @WillAyd let's do the straight split first, modifications later.

ping on green.

@jreback jreback added this to the 0.25.0 milestone Jul 9, 2019
@jreback
Copy link
Contributor

jreback commented Jul 10, 2019

I think we merged something, can you merge master and ping on green.

@mroeschke
Copy link
Member Author

All green @jreback

@jreback jreback merged commit 50fb400 into pandas-dev:master Jul 10, 2019
@jreback
Copy link
Contributor

jreback commented Jul 10, 2019

thanks @mroeschke

@mroeschke mroeschke deleted the clean_window_tests branch July 10, 2019 18:41
@simonjayhawkins
Copy link
Member

it looks like this broke the ci.

@jreback
Copy link
Contributor

jreback commented Jul 10, 2019

@simonjayhawkins can u put a link up

@simonjayhawkins
Copy link
Member

@ghost
Copy link

ghost commented Jul 10, 2019

Strange that my PRs based on earlier commit also fails. Does azure merge the PR into master before tesing?

@simonjayhawkins
Copy link
Member

Does azure merge the PR into master before tesing?

it looks like it. good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Testing pandas testing functions or related to the test suite Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants