-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PKG: Exclude data test files. #19535
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
Changes from 19 commits
4d77cd8
270e442
26e9b4b
1804bcc
7022152
080f000
151ffda
d9d6570
5849591
31fb0b6
9193f15
e897f11
9cf30fd
95cde7a
10ddddc
e1ea208
8616878
77bf77c
156e14b
f3f3662
2cd9706
aac3606
762a2d1
7c44b77
ad09951
6f02d6b
489f540
8b42d1c
ee4fefd
bac438c
84ccdbf
c4802db
7fd7660
c187f8b
632a61d
b5b70c7
9954bba
c771885
dd75270
dbe0c57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import os | ||
|
||
import pytest | ||
|
||
|
||
@pytest.fixture | ||
def datapath(request): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jorisvandenbossche @jreback thoughts on this approach? The idea would be to replace all instances of hard-coded paths with a call to this. It inspects the configuration and skips the file isn't there and skips are allowed (controlled by the I'm not sure if I could do this with Do we know if any tests actually use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I suppose this is just to figure out the "current file" of the test? So we can use relative paths instead of absolute (eg you changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I should have been clearer. I was wondering if there was a base class somewhere, and two children classes inheriting from that base class, so that But indeed, when we move test files to different directories, we'll need to update the paths. I think that's fine since it'll be a simple find / replace. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should go in the conftest 1 level up |
||
"""Get the path to a data file. | ||
|
||
Parameters | ||
---------- | ||
path : str | ||
Path to the file, relative to ``pandas/tests/`` | ||
|
||
Returns | ||
------- | ||
path : path including ``pandas/tests``. | ||
|
||
Raises | ||
------ | ||
ValueError | ||
If the path doesn't exist and the --strict-data-files option is set. | ||
""" | ||
def deco(*args): | ||
path = os.path.join('pandas', 'tests', *args) | ||
if not os.path.exists(path): | ||
if request.config.getoption("--strict-data-files"): | ||
msg = "Could not find file {} and --strict-data-files is set." | ||
raise ValueError(msg.format(path)) | ||
else: | ||
msg = "Could not find {}." | ||
pytest.skip(msg.format(path)) | ||
return path | ||
return deco |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,8 +37,9 @@ | |
|
||
class TestPandasContainer(object): | ||
|
||
def setup_method(self, method): | ||
self.dirpath = tm.get_data_path() | ||
@pytest.fixture(scope="function", autouse=True) | ||
def setup(self, datapath): | ||
self.dirpath = datapath("io", "json", "data") | ||
|
||
self.ts = tm.makeTimeSeries() | ||
self.ts.name = 'ts' | ||
|
@@ -59,7 +60,8 @@ def setup_method(self, method): | |
self.mixed_frame = _mixed_frame.copy() | ||
self.categorical = _cat_frame.copy() | ||
|
||
def teardown_method(self, method): | ||
yield | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I learned that autouse fixtures can include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the other hand, it also makes this more "complex" to understand IMO (the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've done this myself a few times so wanted to chime in. This approach is more in line with how pytest suggests doing setup/teardown (see here) so I think that's a +1 for it. It also gives you potential visibility into the context of the yield tests (ex: here I think you could replace the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, that's a good reason to use an autouse fixture here in this case. (you don't need to convince me of the benefit of fixtures in general :), however, I think many people are not that familiar with all those pytest special features and it has a steeper learning curve IMO, so there can be a balance in how fancy we go) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. When documenting this, I'll recommend against autouse for cases like this. I think it'd be better to just have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah we should really not use this pattern, rather changing to all fixtures. as a temporary workaround this ok, can you create an issue to 'fix' this properly though. |
||
|
||
del self.dirpath | ||
|
||
del self.ts | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe should make a variable that holds all of these options for both the echo and the run to avoid duplication