Split test_excel.py (#24472)
State: closed

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 #24472 
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Duplicate Report
description: Duplicate issue or pull request
name: IO Excel
description: read_excel, to_excel
name: Clean

Comments:
Hello @stevenbw! Thanks for updating the PR.







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

##### Comment last updated on January 20, 2019 at 11:34 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=h1) Report
> Merging [#24749](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/33f91d8f9f2e84f2b5f3ac3f0481b691c977c427?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24749/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=tree)

```diff
@@           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% <ø> (ø)` | :arrow_up: |
| #single | `43.07% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=footer). Last update [33f91d8...f6309d9](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=h1) Report
> Merging [#24749](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/33f91d8f9f2e84f2b5f3ac3f0481b691c977c427?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24749/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=tree)

```diff
@@            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% <ø> (ø)` | :arrow_up: |
| #single | `43.08% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24749/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.19% <0%> (+0.18%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=footer). Last update [33f91d8...05467ea](https://codecov.io/gh/pandas-dev/pandas/pull/24749?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@stevenbw The files will need to be named test_base.py and test_xlrd.py
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?
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.
@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?
@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`.
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
Yes I will take a look, should be able to mimic what you have done.
@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