Excel Tests Continued Fixture Cleanup
State: closed

Follow up to #26579

Removing the frame2 fixture which is sparsely used. I've also removed inheritance of `ReadingTestsBase` and made that it's own discoverable class. Previously these tests were only being generated when `TestXlrdReader` was being run which tightly couples reading with xlrd. 

The goal here in subsequent PRs would be to better parametrize `TestReaders` for new engines that come in and truly make `TestXlrdReader` only deal with xlrd specific tests (will have to move around some tests to make that happen)


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

Comments:
This was causing failures for me locally per this convo:

https://github.com/pandas-dev/pandas/pull/26657#issuecomment-499081131

Let's see what happens here...
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26662?src=pr&el=h1) Report
> Merging [#26662](https://codecov.io/gh/pandas-dev/pandas/pull/26662?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/e0c41f79104c5bc61952c9a14f1883cd5bda53f7?src=pr&el=desc) will **decrease** coverage by `50.09%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #26662      +/-   ##
==========================================
- Coverage   91.87%   41.78%   -50.1%     
==========================================
  Files         174      174              
  Lines       50663    50663              
==========================================
- Hits        46548    21168   -25380     
- Misses       4115    29495   +25380
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.78% <ø> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26662?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.37%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.16%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.1%)` | :arrow_down: |
| [pandas/core/sparse/scipy\_sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL3NjaXB5X3NwYXJzZS5weQ==) | `10.14% <0%> (-89.86%)` | :arrow_down: |
| [pandas/core/tools/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvbnVtZXJpYy5weQ==) | `10.14% <0%> (-89.86%)` | :arrow_down: |
| ... and [128 more](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26662?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/26662?src=pr&el=footer). Last update [e0c41f7...417fc30](https://codecov.io/gh/pandas-dev/pandas/pull/26662?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/26662?src=pr&el=h1) Report
> Merging [#26662](https://codecov.io/gh/pandas-dev/pandas/pull/26662?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/cf25c5cd3ab1f647f780167df622c74b737fa8f5?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #26662      +/-   ##
==========================================
- Coverage   91.78%   91.77%   -0.01%     
==========================================
  Files         174      174              
  Lines       50703    50703              
==========================================
- Hits        46538    46534       -4     
- Misses       4165     4169       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.37% <ø> (ø)` | :arrow_up: |
| #single | `41.81% <ø> (-0.09%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26662?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26662/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.88% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26662?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/26662?src=pr&el=footer). Last update [cf25c5c...fd697ea](https://codecov.io/gh/pandas-dev/pandas/pull/26662?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lgtm. @simonjayhawkins 
@WillAyd changes look good.

test failure is a mystery to me at the moment. I would say it was unrelated if it wasn't for the fact that no other builds are seeing this.

the failing test has an engine parameter. coincidence? no reason that changes to test_excel should affect this test. test output shows `engine = 'numexpr'` so implies no clash of fixtures.

the parametrisation of the engine fixture in pandas\tests\computation\test_eval.py does look iffy though.
Yea no rush to merge I’ll take a look and maybe merge master. Seems unlikely but a clash in fixture naming could be possible. 

Sent from my iPhone

> On Jun 7, 2019, at 5:31 AM, Simon Hawkins <notifications@github.com> wrote:
> 
> @WillAyd changes look good.
> 
> test failure is a mystery to me at the moment. I would say it was unrelated if it wasn't for the fact that no other builds are seeing this.
> 
> the failing test has an engine parameter. coincidence? no reason that changes to test_excel should affect this test. test output shows engine = 'numexpr' so implies no clash of fixtures.
> 
> the parametrisation of the engine fixture in pandas\tests\computation\test_eval.py does look iffy though.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

i think the engine bit may be a coincidence.

it appears that a few tests are using set_use_numexpr which is messing with global variables instead of monkeypatching.

so if the order of tests change, could be any unrelated changes, this could cause a problem.

as an experiment, you could try changing the engine fixture in pandas\tests\computation\test_eval.py to

```
@pytest.fixture(params=_engines)
def engine(request):
    engine = request.param
    if engine == 'numexpr' and not _USE_NUMEXPR:
        pytest.skip('numexpr enabled->{enabled}, '
                    'installed->{installed}'.format(
                        enabled=_USE_NUMEXPR,
                        installed=_NUMEXPR_INSTALLED))
    return engine
```

so that the skip is evaluated at test execution instead of during test collection since skipping depends on the global variable.


Makes sense and I think what you have is more readable anyway. Just pushed
i have a suspicion that'll probably not work because the test is quite involved so there will still be lag between checking the global and performing the failing op.

not sure how much time you want to spend on this. could just revert the last couple of commits for now.

i've had a go a parametrising the tests to split them up. but it may be easier to just debug the test on ci by adding print statements.

```
    def _test_basic_series_frame_alignment(
            self, engine, parser, r_idx_type, c_idx_type, index_name):
        with warnings.catch_warnings(record=True):
            # avoid warning about comparing strings and ints
            warnings.simplefilter("ignore", RuntimeWarning)
            df = mkdf(10, 7, data_gen_f=f, r_idx_type=r_idx_type,
                      c_idx_type=c_idx_type)
            index = getattr(df, index_name)
            s = Series(np.random.randn(5), index[:5])
            if should_warn(s.index, df.index):
                with tm.assert_produces_warning(RuntimeWarning):
                    res = pd.eval('s + df', engine=engine, parser=parser)
            else:
                res = pd.eval('s + df', engine=engine, parser=parser)

            if r_idx_type == 'dt' or c_idx_type == 'dt':
                expected = df.add(s) if engine == 'numexpr' else s + df
            else:
                expected = s + df
            assert_frame_equal(res, expected)

    @pytest.mark.parametrize('r_idx_type', ['i', 'u', 's'])
    @pytest.mark.parametrize('c_idx_type', ['i', 'u', 's'])
    @pytest.mark.parametrize('index_name', ['index', 'columns'])
    def test_basic_series_frame_alignment(
            self, engine, parser, r_idx_type, c_idx_type, index_name):
        self._test_basic_series_frame_alignment(
            engine, parser, r_idx_type, c_idx_type, index_name)

    @pytest.mark.parametrize('index_name', ['index', 'columns'])
    def test_basic_series_frame_alignment_dt(self, engine, parser, index_name):
        # only test dt with dt, otherwise weird joins result
        self._test_basic_series_frame_alignment(
            engine, parser, 'dt', 'dt', index_name)```

OK yea let me revert the last few changes then. Even if it passes CI somewhat fragile and orthogonal anyway. PR is big enough as is so happy to address separately and in coordination with whatever you are looking at
Thanks for the review @simonjayhawkins