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

- [ ] closes #24472 and update to #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.


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: IO Excel
description: read_excel, to_excel

Comments:
Most of the failures appear to because of the use of the depreciated `parse_cols` instead of `usecols`.
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.
> 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?
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...
Thanks @WillAyd , @jreback ! I will ensure I pull the latest changes and take a look over the weekend.
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 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 <notifications@github.com> 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.

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 

``` shell
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
```
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?
@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`.
would love to have this. probably better to start over with a clean copy of master.