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

[stevenbw]

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

In brief, I have tried to split the quite large test_excel.py test file into logical chunks. I have done this by first splitting by reading and writing, and then by engine.

[stevenbw]

Most of the failures appear to because of the use of the depreciated parse_cols instead of usecols.

[jreback]

is this a direct split w/o actually changing anything? we much prefer this, its impossible to test what you actually change vs just moved.

[stevenbw]

Yes I have only moved code into separate files and not made any changes. This was intentional since, as you said, it cannot be tested and also I'd rather push small changes, rather than rearranging and changing code in one PR.

I checked the original test_excel.py file, this also uses the parse_cols argument. I presume this functionality has depreciated since the development of these tests.

[jreback]

Most of the failures appear to because of the use of the depreciated parse_cols instead of usecols

this was quite a while ago, are you sure you started with current master?

[WillAyd]

If you can merge master there were some changes made since you opened this PR would need to be split.

Generally I'd comment that I think the sub-directories for reader and writer are a little overkill; we had a similar conversation on the actual splitting of code and the only engine that can really do both in theory is openpyxl, but that's not even implemented at this point

If I could suggest an alternate structure it would probably look something like:

• common.py (for SharedItems class)
• test_readers.py
• test_writers.py
• test_{engine}.py for tests specific to the various engines
• test_style.py for styling tests (maybe also in test_writers, not sure)

This is a rather large document to split up and it doesn't need to all be done at once. In fact it may be preferable to do multiple PRs where you split reading tests first, then writing, etc...

[stevenbw]

Thanks @WillAyd , @jreback ! I will ensure I pull the latest changes and take a look over the weekend.

[stevenbw]

Hi @WillAyd , I have fetched the latest changes from master and then split test_excel.py into three for now as you suggested into common.py, test_reader.py and test_writer.py. However when I run the test_reader.py test module it results in several failures due to the depreciation of 'parse_cols'. I checked test_excel.py at master, that too contains the 'parse_cols' argument. Is this performing correctly? Is it testing to see if a depreciated argument is used? Thanks

[WillAyd]

You aren’t getting that error before moving right? Don’t think splitting should affect that so ensure that first but if not push your updates can take a look from there next week

…

Sent from my iPhone

On Feb 22, 2019, at 7:30 AM, Steven Beardwell ***@***.***> wrote: Hi @WillAyd , I have fetched the latest changes from master and then split test_excel.py into three for now as you suggested into common.py, test_reader.py and test_writer.py. However when I run the test_reader.py test module it results in several failures due to the depreciation of 'parse_cols'. I checked test_excel.py at master, that too contains the 'parse_cols' argument. Is this performing correctly? Is it testing to see if a depreciated argument is used? Thanks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[stevenbw]

There is no error before the split. Could it be realated to the stack_level? The function is executed in ReadingTestsBase in test_reader.py , using arguments from SharedItems , defined in common.py.

The error is

AssertionError: Warning not set with correct stacklevel. File where warning is raised: pandas\tests\io\excel\common.py != pandas\tests\io\excel\test_reader.py. Warning message: the 'parse_cols' keyword is deprecated, use 'usecols' instead

[WillAyd]

Hmm I see there isn't consistency in existing codebase around stacklevel handling so we should maybe as a precursor here make that consistent with stacklevel=False before moving or alternately just use pytest.warns

@gfyoung I think you've been looking a lot at leveraging pytest methods instead of internal testing ones - any thoughts to doing that here?

[gfyoung]

@WillAyd : The general consensus is continue using tm.assert_produces_warning because of the check for stacklevel. That being said, if you're having issues getting the right level, pass in check_stacklevel=False.

[jreback]

would love to have this. probably better to start over with a clean copy of master.