Split test_excel.py (#24472) #24749

stevenbw · 2019-01-13T11:25:26Z

Split tests/io/test_excel.py into two files, base.py and xlrd.py located in tests/io/excel/. The previous file was very large and a logical split has been applied to separate the xlrd tests.

closes TST: Split test_excel into sub test files #24472
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2019-01-13T11:25:29Z

Hello @stevenbw! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 20, 2019 at 11:34 Hours UTC

codecov · 2019-01-13T12:00:48Z

Codecov Report

Merging #24749 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24749   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52358    52358           
=======================================
  Hits        48373    48373           
  Misses       3985     3985

Flag	Coverage Δ
#multiple	`90.81% <ø> (ø)`	⬆️
#single	`43.07% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f91d8...f6309d9. Read the comment docs.

codecov · 2019-01-13T12:00:48Z

Codecov Report

Merging #24749 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #24749      +/-   ##
==========================================
+ Coverage   92.38%   92.39%   +<.01%     
==========================================
  Files         166      166              
  Lines       52358    52358              
==========================================
+ Hits        48373    48375       +2     
+ Misses       3985     3983       -2

Flag	Coverage Δ
#multiple	`90.81% <ø> (ø)`	⬆️
#single	`43.08% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/util/testing.py	`88.19% <0%> (+0.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f91d8...05467ea. Read the comment docs.

simonjayhawkins · 2019-01-13T14:47:18Z

@stevenbw The files will need to be named test_base.py and test_xlrd.py

simonjayhawkins · 2019-01-13T17:33:02Z

pandas/tests/io/excel/test_base.py

@@ -128,96 +77,6 @@ def set_engine(self, request):
        yield
        setattr(self, func_name, old_func)

-    @td.skip_if_no("xlrd", "1.0.1")  # see gh-22682
-    def test_usecols_int(self, ext):


you'll probably just want to split by complete classes in this PR and leave refactoring the classes to a follow-on PR.

simonjayhawkins · 2019-01-13T17:33:42Z

pandas/tests/io/excel/test_xlrd.py

+
+
+@contextlib.contextmanager
+def ignore_xlrd_time_clock_warning():


any code required by both test_base.py and test_xlrd.py should probably be in test_base.py

simonjayhawkins · 2019-01-13T18:19:49Z

pandas/tests/io/excel/test_xlrd.py

+class XlrdReadingTestsBase(XlrdSharedItems):
+    # This is based on ExcelWriterBase
+
+    @pytest.fixture(autouse=True, params=['xlrd'])


code duplicated from test_base

simonjayhawkins · 2019-01-13T18:20:46Z

pandas/tests/io/excel/test_base.py


 class ReadingTestsBase(SharedItems):
    # This is based on ExcelWriterBase

-    @pytest.fixture(autouse=True, params=['xlrd', None])
+    @pytest.fixture(autouse=True, params=[None])


maybe a better name for test_base would be test_common or test_all_engines. this split is creating too much code duplication. maybe a split on reader/writer tests first before splitting out xlrd

WillAyd · 2019-01-13T20:43:11Z

Agreed high level with @simonjayhawkins think there are just too many things moved to test_xlrd. If anything the entire SharedItems class you have can remain in the base file.

Did this yield the same number of tests before and after?

jreback

so before this is actually split; it needs to be fixturized a bit more I think. not really sure the best way of doing this, but if you are just moving files, then that can be in a single PR. We don't want to mix moving things with changing things as its very hard to tell what is going on. So first move w/o any changes, then new PR' to change.

stevenbw · 2019-01-14T20:32:21Z

Thanks for the support @simonjayhawkins, @WillAyd, @jreback :). I have taken on board your comments and attempted to simply split the file without separating classes or duplicating code. However, due to classes relying on previous classes, for example TestXlrdReader, which could belong to test_xlrd.py requires ReadingTestsBase, which belongs to test_all_engines.py, there is only a small amount of code which can be moved. Other than TestXlrdReader there is only ignore_xlrd_time_clock_warning which could be in a separate file but it is used several times in ReadingTestsBase, it could be imported but doesn't seem to be of much benefit.

Unless I am missing something, it seems the ideal solution would be to rewrite the classes so they can be decoupled. I appreciate this would be a big change though.

simonjayhawkins · 2019-01-14T21:45:34Z

@stevenbw the biggest classes by far are ReadingTestsBase and TestExcelWriter. i think these would need to be in separate files in order to make an impact on reducing the test file size. could the classes be grouped for this to work?

stevenbw · 2019-01-15T17:16:12Z

@simonjayhawkins Yes they could. They both have a fundamental dependency on SharedItems but one option could be to import this class, then have one file test_reader.py for ReadingTestsBase and TestXlrdReder. The other file could contain all other classes and be called test_writer.py.

WillAyd · 2019-02-12T05:35:48Z

Can you merge master here? Also we just refactored the excel code in #25153 so you may be able to mimic that set up on the test side

stevenbw · 2019-02-13T20:39:05Z

Yes I will take a look, should be able to mimic what you have done.

WillAyd · 2019-03-09T20:09:50Z

@stevenbw I think this is a duplicate of #25334 but if that's not the case let me know.

Generally should just keep pushing updates to the same PR rather than opening a new one

Split test_excel.py (pandas-dev#24472)

f6309d9

Split test_excel.py (pandas-dev#24472)

0f9d7f6

stevenbw added 3 commits January 13, 2019 15:20

Split test_excel.py (pandas-dev#24472)

b21b2aa

Split test_excel.py (pandas-dev#24472)

11ee17e

Split test_excel.py (pandas-dev#24472)

1c2cd2c

simonjayhawkins reviewed Jan 13, 2019

View reviewed changes

WillAyd added Testing pandas testing functions or related to the test suite IO Excel read_excel, to_excel Clean labels Jan 13, 2019

jreback requested changes Jan 13, 2019

View reviewed changes

TST: Split test_excel.py (pandas-dev#24472)

05467ea

simonjayhawkins mentioned this pull request Feb 5, 2019

Split Excel IO Into Sub-Directory #25153

Merged

stevenbw mentioned this pull request Feb 15, 2019

TST: Split test_excel.py into sub test files #24472 #25334

Closed

4 tasks

WillAyd closed this Mar 9, 2019

WillAyd added the Duplicate Report Duplicate issue or pull request label Mar 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split test_excel.py (#24472) #24749

Split test_excel.py (#24472) #24749

stevenbw commented Jan 13, 2019

pep8speaks commented Jan 13, 2019 •

edited

Loading

codecov bot commented Jan 13, 2019

codecov bot commented Jan 13, 2019 •

edited

Loading

simonjayhawkins commented Jan 13, 2019

simonjayhawkins Jan 13, 2019

simonjayhawkins Jan 13, 2019

simonjayhawkins Jan 13, 2019

simonjayhawkins Jan 13, 2019

WillAyd commented Jan 13, 2019

jreback left a comment

stevenbw commented Jan 14, 2019

simonjayhawkins commented Jan 14, 2019

stevenbw commented Jan 15, 2019

WillAyd commented Feb 12, 2019

stevenbw commented Feb 13, 2019

WillAyd commented Mar 9, 2019



		@contextlib.contextmanager
		def ignore_xlrd_time_clock_warning():

Split test_excel.py (#24472) #24749

Split test_excel.py (#24472) #24749

Conversation

stevenbw commented Jan 13, 2019

pep8speaks commented Jan 13, 2019 • edited Loading

Comment last updated on January 20, 2019 at 11:34 Hours UTC

codecov bot commented Jan 13, 2019

Codecov Report

codecov bot commented Jan 13, 2019 • edited Loading

Codecov Report

simonjayhawkins commented Jan 13, 2019

simonjayhawkins Jan 13, 2019

Choose a reason for hiding this comment

simonjayhawkins Jan 13, 2019

Choose a reason for hiding this comment

simonjayhawkins Jan 13, 2019

Choose a reason for hiding this comment

simonjayhawkins Jan 13, 2019

Choose a reason for hiding this comment

WillAyd commented Jan 13, 2019

jreback left a comment

Choose a reason for hiding this comment

stevenbw commented Jan 14, 2019

simonjayhawkins commented Jan 14, 2019

stevenbw commented Jan 15, 2019

WillAyd commented Feb 12, 2019

stevenbw commented Feb 13, 2019

WillAyd commented Mar 9, 2019

pep8speaks commented Jan 13, 2019 •

edited

Loading

codecov bot commented Jan 13, 2019 •

edited

Loading