-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Cleanup clipboard tests #21163
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
Cleanup clipboard tests #21163
Conversation
Hello @david-liu-brattle-1! Thanks for updating the PR.
Comment last updated on June 23, 2018 at 19:07 Hours UTC |
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.
so what you can do is xfail the cases which don't work now but will work in the new PR.
pandas/tests/io/test_clipboard.py
Outdated
tm.assert_frame_equal(data, result, check_dtype=False) | ||
|
||
def test_round_trip_frame(self): |
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.
this can be parameterized or a fixture
pandas/tests/io/test_clipboard.py
Outdated
@@ -60,6 +60,9 @@ def setup_class(cls): | |||
# unicode round trip test for GH 13747, GH 12529 | |||
cls.data['utf8'] = pd.DataFrame({'a': ['µasd', 'Ωœ∑´'], |
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.
can remove this entirely and use fixtures
pandas/tests/io/test_clipboard.py
Outdated
def test_round_trip_frame(self): | ||
for dt in self.data_types: | ||
self.check_round_trip_frame(dt) | ||
|
||
def test_round_trip_frame_sep(self): |
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.
these can all be parameterized
pandas/tests/io/test_clipboard.py
Outdated
with tm.assert_produces_warning(): | ||
self.data['string'].to_clipboard(excel=True, sep=r'\t') | ||
|
||
def build_kwargs(self, sep, excel): |
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.
you can make this a module level function
pandas/tests/io/test_clipboard.py
Outdated
(None, False), | ||
('default', False) | ||
]) | ||
def test_clipboard_copy_strings(self, sep, excel): |
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.
can parameterize over data_types as well
@jreback |
Codecov Report
@@ Coverage Diff @@
## master #21163 +/- ##
==========================================
+ Coverage 91.84% 91.9% +0.06%
==========================================
Files 153 154 +1
Lines 49505 49562 +57
==========================================
+ Hits 45466 45549 +83
+ Misses 4039 4013 -26
Continue to review full report at Codecov.
|
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.
do all of these tests actually need xfailing?
pandas/tests/io/test_clipboard.py
Outdated
with tm.assert_produces_warning(): | ||
gen_df('string').to_clipboard(excel=True, sep=r'\t') | ||
|
||
@pytest.mark.xfail |
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.
for tests that xfail, pls provide a reason
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.
Should I add comments in the code with the reason?
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.
yes if they are xfailing, reference the issue number and a brief description. (when the bug is fixed will remove the xfails, but that's in a subsequent PR).
pandas/tests/io/test_clipboard.py
Outdated
def test_round_trip_frame(self, df): | ||
self.check_round_trip_frame(df) | ||
|
||
def test_round_trip_frame_sep(self, df): |
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.
you can parameterize on the sep
pandas/tests/io/test_clipboard.py
Outdated
@pytest.fixture(params=['delims', 'utf8', 'string', 'long', 'nonascii', | ||
'colwidth', 'mixed', 'float', 'int']) | ||
def df(request): | ||
return gen_df(request.param) |
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.
I would just move gen_df inside the fixture itself, I think makes it more clear.
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.
gen_df is called directly from some of the tests. The fixture is called using a a pytest object and gen_df needs to be called with a string argument. Is there a better workaround than separating the two functions?
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.
I can change it to something like this, but I think separating the two out is still preferable.
@pytest.fixture(params=['delims', 'utf8', 'string', 'long', 'nonascii',
'colwidth', 'mixed', 'float', 'int'])
def df(request):
if type(request) is str:
data_type = request
else:
data_type = request.param
if data_type == 'delims':
return pd.DataFrame({'a': ['"a,\t"b|c', 'd\tef´'],
'b': ['hi\'j', 'k\'\'lm']})
elif data_type == 'utf8':
return pd.DataFrame({'a': ['µasd', 'Ωœ∑´'],
'b': ['øπ∆˚¬', 'œ∑´®']})
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.
oh didn't see that, though then this begs the question of why you are not simply passing the constructed df in?
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.
That's a good point, I'll update
pandas/tests/io/test_clipboard.py
Outdated
# delimited and excel="True" | ||
# Fails because to_clipboard defaults to space delim. | ||
# Issue in #21104, Fixed in #21111 | ||
@pytest.mark.xfail |
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.
every xfail needs a reason
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.
Can you take a look at the latest version? Is that what you meant for reason?
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.
minor comment. all of the currently xfailed ones are fixed by #21111 ?
pandas/tests/io/test_clipboard.py
Outdated
@pytest.mark.xfail(reason="Not yet implemented. Fixed in #21111") | ||
def test_excel_sep_warning(self): | ||
with tm.assert_produces_warning(): | ||
gen_df('string').to_clipboard(excel=True, sep=r'\t') |
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.
so instead of calling gen_df inside a test, just create a nother fixture, simplier that way I think
e.g.
@pytest.fixture
def df_string():
return gen_df('string')
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.
Thanks, that makes a lot of sense and I'll keep that in mind for future cases. I went a different way and just ran that test function on the entire df
fixture, which is a little cleaner.
thanks @david-liu-brattle-1 ! |
(cherry picked from commit 9d38e0e)
(cherry picked from commit 9d38e0e)
As requested in #21111, I've refactored the original clipboard tests as well as adding my own. The set of tests have been improved to include testing of:
As a result, these tests will fail on the current master branch, but should succeed on the commit in #21111
@WillAyd