Excel Tests Continued Fixture Cleanup

[WillAyd]

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)

[WillAyd]

Note that our project-wide minimum ix 1.0.0, so this decorator was being duplicative.

This decorator also won't be there in the long term once we completely couple TestReaders from xlrd, but keeping here for now

[WillAyd]

This was causing failures for me locally per this convo:

#26657 (comment)

Let's see what happens here...

[codecov]

Codecov Report

Merging #26662 into master will decrease coverage by 50.09%.
The diff coverage is n/a.

@@            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%)	⬇️

Impacted Files	Coverage Δ
pandas/io/formats/latex.py	0% <0%> (-100%)	⬇️
pandas/io/sas/sas_constants.py	0% <0%> (-100%)	⬇️
pandas/core/groupby/categorical.py	0% <0%> (-100%)	⬇️
pandas/tseries/plotting.py	0% <0%> (-100%)	⬇️
pandas/tseries/converter.py	0% <0%> (-100%)	⬇️
pandas/io/formats/html.py	0% <0%> (-99.37%)	⬇️
pandas/io/sas/sas7bdat.py	0% <0%> (-91.16%)	⬇️
pandas/io/sas/sas_xport.py	0% <0%> (-90.1%)	⬇️
pandas/core/sparse/scipy_sparse.py	10.14% <0%> (-89.86%)	⬇️
pandas/core/tools/numeric.py	10.14% <0%> (-89.86%)	⬇️
... and 128 more

---

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 e0c41f7...417fc30. Read the comment docs.

[codecov]

Codecov Report

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

@@            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% <ø> (ø)	⬆️
#single	41.81% <ø> (-0.09%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	78.94% <0%> (-10.53%)	⬇️
pandas/core/frame.py	96.88% <0%> (-0.12%)	⬇️

---

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 cf25c5c...fd697ea. Read the comment docs.

[WillAyd]

OK think I'm stopping here. This should be a pretty good cleanup to remove unnecessary fixturation.

I think after this going to align the engine / ext usage in the Writer classes and can start splitting into sub-modules thereafter

[WillAyd]

These didn't really solve the issue they were supposed to so removed to make things cleaner. Again I think the real issue occurs when defusedxml is installed in the right environment, but will have to delve further into which environments that is exactly (orthogonal to this PR)

[WillAyd]

Comment might have been lost on a prior PR but the min xlrd version for pandas is 1.0.0 anyway, so this was duplicative

[jreback]

lgtm. @simonjayhawkins

[simonjayhawkins]

@WillAyd generally lgtm. a few suggestions.

[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.

[WillAyd]

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 ***@***.***> 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.

[simonjayhawkins]

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.

[WillAyd]

Makes sense and I think what you have is more readable anyway. Just pushed

[simonjayhawkins]

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)```

[WillAyd]

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

[WillAyd]

Thanks for the review @simonjayhawkins