Repository: pandas-dev/pandas
Pull request: TST: Decoupled more xlrd reading tests from openpyxl
- [X] closes #27111
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

@simonjayhawkins 

Problem statement:
TST: openpyxl tests fail if xlrd is not installed
If you run the `tests/io/excel/test_readers.py` with only openpyxl installed, the xlrd tests are skipped, but some of the openpyxl tests still fail on needing xlrd.

eg:
```
____________________________________________________________ TestExcelFileRead.test_read_excel_engine_value[openpyxl-.xlsm-None] _____________________________________________________________

self = <pandas.tests.io.excel.test_readers.TestExcelFileRead object at 0x7fca3bb6d198>, read_ext = '.xlsm', excel_engine = None

    @pytest.mark.parametrize('excel_engine', [
        'xlrd',
        None
    ])
    def test_read_excel_engine_value(self, read_ext, excel_engine):
        # GH 26566
>       xl = ExcelFile("test1" + read_ext, engine=excel_engine)

../excel/test_readers.py:853: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../io/excel/_base.py:791: in __init__
    self._reader = self._engines[engine](self._io)
../../../io/excel/_xlrd.py:21: in __init__
    import_optional_dependency("xlrd", extra=err_msg)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

name = 'xlrd', extra = 'Install xlrd >= 1.0.0 for Excel support', raise_on_missing = True, on_version = 'raise'

    def import_optional_dependency(
        name: str,
        extra: str = "",
        raise_on_missing: bool = True,
        on_version: str = "raise",
    ):
        """
        Import an optional dependency.
    
        By default, if a dependency is missing an ImportError with a nice
        message will be raised. If a dependency is present, but too old,
        we raise.
    
        Parameters
        ----------
        name : str
            The module name. This should be top-level only, so that the
            version may be checked.
        extra : str
            Additional text to include in the ImportError message.
        raise_on_missing : bool, default True
            Whether to raise if the optional dependency is not found.
            When False and the module is not present, None is returned.
        on_version : str {'raise', 'warn'}
            What to do when a dependency's version is too old.
    
            * raise : Raise an ImportError
            * warn : Warn that the version is too old. Returns None
            * ignore: Return the module, even if the version is too old.
              It's expected that users validate the version locally when
              using ``on_version="ignore"`` (see. ``io/html.py``)
    
        Returns
        -------
        maybe_module : Optional[ModuleType]
            The imported module, when found and the version is correct.
            None is returned when the package is not found and `raise_on_missing`
            is False, or when the package's version is too old and `on_version`
            is ``'warn'``.
        """
        try:
            module = importlib.import_module(name)
        except ImportError:
            if raise_on_missing:
>               raise ImportError(message.format(name=name, extra=extra)) from None
E               ImportError: Missing optional dependency 'xlrd'. Install xlrd >= 1.0.0 for Excel support Use pip or conda to install xlrd.
```

The actual excel reading (interactively) is not affected, so certainly not a blocking issue.

Hints from issue discussion:

Commit message: Decoupled xlrd tests from openpyxl

Patch to review:
diff --git a/pandas/tests/io/excel/test_readers.py b/pandas/tests/io/excel/test_readers.py
index 579f39e21d3c1..31fd5a2be74c9 100644
--- a/pandas/tests/io/excel/test_readers.py
+++ b/pandas/tests/io/excel/test_readers.py
@@ -15,7 +15,6 @@
 import pandas.util.testing as tm
 
 from pandas.io.common import URLError
-from pandas.io.excel import ExcelFile
 
 
 @contextlib.contextmanager
@@ -736,17 +735,20 @@ class TestExcelFileRead:
         pytest.param(None, marks=pytest.mark.skipif(
             not td.safe_import("xlrd"), reason="no xlrd")),
     ])
-    def cd_and_set_engine(self, request, datapath, monkeypatch):
+    def cd_and_set_engine(self, request, datapath, monkeypatch, read_ext):
         """
         Change directory and set engine for ExcelFile objects.
         """
+        if request.param == 'openpyxl' and read_ext == '.xls':
+            pytest.skip()
+
         func = partial(pd.ExcelFile, engine=request.param)
         monkeypatch.chdir(datapath("io", "data"))
         monkeypatch.setattr(pd, 'ExcelFile', func)
 
     def test_excel_passes_na(self, read_ext):
 
-        excel = ExcelFile('test4' + read_ext)
+        excel = pd.ExcelFile('test4' + read_ext)
 
         parsed = pd.read_excel(excel, 'Sheet1', keep_default_na=False,
                                na_values=['apple'])
@@ -754,6 +756,7 @@ def test_excel_passes_na(self, read_ext):
                              columns=['Test'])
         tm.assert_frame_equal(parsed, expected)
 
+        excel = pd.ExcelFile('test4' + read_ext)
         parsed = pd.read_excel(excel, 'Sheet1', keep_default_na=True,
                                na_values=['apple'])
         expected = DataFrame([[np.nan], [1], [np.nan], [np.nan], ['rabbit']],
@@ -761,7 +764,7 @@ def test_excel_passes_na(self, read_ext):
         tm.assert_frame_equal(parsed, expected)
 
         # 13967
-        excel = ExcelFile('test5' + read_ext)
+        excel = pd.ExcelFile('test5' + read_ext)
 
         parsed = pd.read_excel(excel, 'Sheet1', keep_default_na=False,
                                na_values=['apple'])
@@ -769,6 +772,7 @@ def test_excel_passes_na(self, read_ext):
                              columns=['Test'])
         tm.assert_frame_equal(parsed, expected)
 
+        excel = pd.ExcelFile('test5' + read_ext)
         parsed = pd.read_excel(excel, 'Sheet1', keep_default_na=True,
                                na_values=['apple'])
         expected = DataFrame([[np.nan], [1], [np.nan], [np.nan], ['rabbit']],
@@ -778,7 +782,7 @@ def test_excel_passes_na(self, read_ext):
     @pytest.mark.parametrize('arg', ['sheet', 'sheetname', 'parse_cols'])
     def test_unexpected_kwargs_raises(self, read_ext, arg):
         # gh-17964
-        excel = ExcelFile('test1' + read_ext)
+        excel = pd.ExcelFile('test1' + read_ext)
 
         kwarg = {arg: 'Sheet1'}
         msg = "unexpected keyword argument `{}`".format(arg)
@@ -787,38 +791,39 @@ def test_unexpected_kwargs_raises(self, read_ext, arg):
 
     def test_excel_table_sheet_by_index(self, read_ext, df_ref):
 
-        excel = ExcelFile('test1' + read_ext)
+        excel = pd.ExcelFile('test1' + read_ext)
 
         df1 = pd.read_excel(excel, 0, index_col=0)
         df2 = pd.read_excel(excel, 1, skiprows=[1], index_col=0)
         tm.assert_frame_equal(df1, df_ref, check_names=False)
         tm.assert_frame_equal(df2, df_ref, check_names=False)
 
+        excel = pd.ExcelFile('test1' + read_ext)
         df1 = excel.parse(0, index_col=0)
         df2 = excel.parse(1, skiprows=[1], index_col=0)
         tm.assert_frame_equal(df1, df_ref, check_names=False)
         tm.assert_frame_equal(df2, df_ref, check_names=False)
 
+        excel = pd.ExcelFile('test1' + read_ext)
         df3 = pd.read_excel(excel, 0, index_col=0, skipfooter=1)
         tm.assert_frame_equal(df3, df1.iloc[:-1])
 
+        excel = pd.ExcelFile('test1' + read_ext)
         with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
             df4 = pd.read_excel(excel, 0, index_col=0, skip_footer=1)
             tm.assert_frame_equal(df3, df4)
 
+        excel = pd.ExcelFile('test1' + read_ext)
         df3 = excel.parse(0, index_col=0, skipfooter=1)
         tm.assert_frame_equal(df3, df1.iloc[:-1])
 
-        import xlrd  # will move to engine-specific tests as new ones are added
-        with pytest.raises(xlrd.XLRDError):
-            pd.read_excel(excel, 'asdf')
-
     def test_sheet_name(self, read_ext, df_ref):
         filename = "test1"
         sheet_name = "Sheet1"
 
-        excel = ExcelFile(filename + read_ext)
+        excel = pd.ExcelFile(filename + read_ext)
         df1_parse = excel.parse(sheet_name=sheet_name, index_col=0)  # doc
+        excel = pd.ExcelFile(filename + read_ext)
         df2_parse = excel.parse(index_col=0,
                                 sheet_name=sheet_name)
 
@@ -826,31 +831,27 @@ def test_sheet_name(self, read_ext, df_ref):
         tm.assert_frame_equal(df2_parse, df_ref, check_names=False)
 
     def test_excel_read_buffer(self, read_ext):
-
         pth = 'test1' + read_ext
-        expected = pd.read_excel(pth, 'Sheet1', index_col=0)
+        engine = pd.ExcelFile.keywords['engine']  # TODO: fixturize
+        expected = pd.read_excel(pth, 'Sheet1', index_col=0, engine=engine)
 
         with open(pth, 'rb') as f:
-            xls = ExcelFile(f)
+            xls = pd.ExcelFile(f)
             actual = pd.read_excel(xls, 'Sheet1', index_col=0)
             tm.assert_frame_equal(expected, actual)
 
-    def test_reader_closes_file(self, read_ext):
-
-        f = open('test1' + read_ext, 'rb')
-        with ExcelFile(f) as xlsx:
-            # parses okay
-            pd.read_excel(xlsx, 'Sheet1', index_col=0)
-
-        assert f.closed
 
-    @pytest.mark.parametrize('excel_engine', [
-        'xlrd',
-        None
-    ])
-    def test_read_excel_engine_value(self, read_ext, excel_engine):
-        # GH 26566
-        xl = ExcelFile("test1" + read_ext, engine=excel_engine)
-        msg = "Engine should not be specified when passing an ExcelFile"
-        with pytest.raises(ValueError, match=msg):
-            pd.read_excel(xl, engine='openpyxl')
+@td.skip_if_no("openpyxl")
+@pytest.mark.parametrize('excel_engine', [
+    pytest.param('xlrd', marks=pytest.mark.skipif(
+        not td.safe_import("xlrd"), reason="no xlrd")),
+    pytest.param(None, marks=pytest.mark.skipif(
+        not td.safe_import("xlrd"), reason="no xlrd")),
+])
+def test_conflicting_excel_engines(read_ext, excel_engine, datapath):
+    # GH 26566
+    path = datapath("io", "data", 'test1{}'.format(read_ext))
+    xl = pd.ExcelFile(path, engine=excel_engine)
+    msg = "Engine should not be specified when passing an ExcelFile"
+    with pytest.raises(ValueError, match=msg):
+        pd.read_excel(xl, engine='openpyxl')
diff --git a/pandas/tests/io/excel/test_xlrd.py b/pandas/tests/io/excel/test_xlrd.py
index b9fc9305a4033..a46e458da4ab8 100644
--- a/pandas/tests/io/excel/test_xlrd.py
+++ b/pandas/tests/io/excel/test_xlrd.py
@@ -27,3 +27,20 @@ def test_read_xlrd_book(read_ext, frame):
         result = pd.read_excel(book, sheet_name=sheet_name,
                                engine=engine, index_col=0)
         tm.assert_frame_equal(df, result)
+
+
+# TODO: test for openpyxl as well
+def test_excel_table_sheet_by_index(datapath, read_ext):
+    excel = pd.ExcelFile(datapath("io", "data", 'test1{}'.format(read_ext)))
+    with pytest.raises(xlrd.XLRDError):
+        pd.read_excel(excel, 'asdf')
+
+
+# TODO: test for openpyxl as well
+def test_reader_closes_file(datapath, read_ext):
+    f = open(datapath("io", "data", 'test1{}'.format(read_ext)), 'rb')
+    with pd.ExcelFile(f, engine='xlrd') as xlsx:
+        # parses okay
+        pd.read_excel(xlsx, 'Sheet1', index_col=0, engine='xlrd')
+
+    assert f.closed


Code review comments:
diff_hunk: @@ -787,70 +791,67 @@ def test_unexpected_kwargs_raises(self, read_ext, arg):
 
     def test_excel_table_sheet_by_index(self, read_ext, df_ref):
 
-        excel = ExcelFile('test1' + read_ext)
+        excel = pd.ExcelFile('test1' + read_ext)
 
         df1 = pd.read_excel(excel, 0, index_col=0)
         df2 = pd.read_excel(excel, 1, skiprows=[1], index_col=0)
         tm.assert_frame_equal(df1, df_ref, check_names=False)
         tm.assert_frame_equal(df2, df_ref, check_names=False)
 
+        excel = pd.ExcelFile('test1' + read_ext)
         df1 = excel.parse(0, index_col=0)
         df2 = excel.parse(1, skiprows=[1], index_col=0)
         tm.assert_frame_equal(df1, df_ref, check_names=False)
         tm.assert_frame_equal(df2, df_ref, check_names=False)
 
+        excel = pd.ExcelFile('test1' + read_ext)
         df3 = pd.read_excel(excel, 0, index_col=0, skipfooter=1)
         tm.assert_frame_equal(df3, df1.iloc[:-1])
 
+        excel = pd.ExcelFile('test1' + read_ext)
         with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
             df4 = pd.read_excel(excel, 0, index_col=0, skip_footer=1)
             tm.assert_frame_equal(df3, df4)
 
+        excel = pd.ExcelFile('test1' + read_ext)
         df3 = excel.parse(0, index_col=0, skipfooter=1)
         tm.assert_frame_equal(df3, df1.iloc[:-1])
 
-        import xlrd  # will move to engine-specific tests as new ones are added
-        with pytest.raises(xlrd.XLRDError):
-            pd.read_excel(excel, 'asdf')
-
     def test_sheet_name(self, read_ext, df_ref):
         filename = "test1"
         sheet_name = "Sheet1"
 
-        excel = ExcelFile(filename + read_ext)
+        excel = pd.ExcelFile(filename + read_ext)
         df1_parse = excel.parse(sheet_name=sheet_name, index_col=0)  # doc
+        excel = pd.ExcelFile(filename + read_ext)
         df2_parse = excel.parse(index_col=0,
                                 sheet_name=sheet_name)
 
         tm.assert_frame_equal(df1_parse, df_ref, check_names=False)
         tm.assert_frame_equal(df2_parse, df_ref, check_names=False)
 
     def test_excel_read_buffer(self, read_ext):
-
         pth = 'test1' + read_ext
-        expected = pd.read_excel(pth, 'Sheet1', index_col=0)
+        engine = pd.ExcelFile.keywords['engine']  # TODO: fixturize
+        expected = pd.read_excel(pth, 'Sheet1', index_col=0, engine=engine)
 
         with open(pth, 'rb') as f:
-            xls = ExcelFile(f)
+            xls = pd.ExcelFile(f)
             actual = pd.read_excel(xls, 'Sheet1', index_col=0)
             tm.assert_frame_equal(expected, actual)
 
-    def test_reader_closes_file(self, read_ext):
-
-        f = open('test1' + read_ext, 'rb')
-        with ExcelFile(f) as xlsx:
-            # parses okay
-            pd.read_excel(xlsx, 'Sheet1', index_col=0)
-
-        assert f.closed
 
-    @pytest.mark.parametrize('excel_engine', [
-        'xlrd',
-        None
-    ])
-    def test_read_excel_engine_value(self, read_ext, excel_engine):
-        # GH 26566
-        xl = ExcelFile("test1" + read_ext, engine=excel_engine)
-        msg = "Engine should not be specified when passing an ExcelFile"
-        with pytest.raises(ValueError, match=msg):
-            pd.read_excel(xl, engine='openpyxl')
+@td.skip_if_no("openpyxl")
+@pytest.mark.parametrize('excel_engine', [
+    pytest.param('xlrd', marks=pytest.mark.skipif(
+        not td.safe_import("xlrd"), reason="no xlrd")),
+    pytest.param(None, marks=pytest.mark.skipif(
+        not td.safe_import("xlrd"), reason="no xlrd")),
+])
+def test_conflicting_excel_engines(read_ext, excel_engine, datapath):
path: pandas/tests/io/excel/test_readers.py
text: @author:
This didn't really follow the same rules for parametrization as the rest of the items in `TestExcelFileRead`so figured clearest to remove from class

diff_hunk: @@ -736,39 +735,44 @@ class TestExcelFileRead:
         pytest.param(None, marks=pytest.mark.skipif(
             not td.safe_import("xlrd"), reason="no xlrd")),
     ])
-    def cd_and_set_engine(self, request, datapath, monkeypatch):
+    def cd_and_set_engine(self, request, datapath, monkeypatch, read_ext):
         """
         Change directory and set engine for ExcelFile objects.
         """
+        if request.param == 'openpyxl' and read_ext == '.xls':
+            pytest.skip()
+
         func = partial(pd.ExcelFile, engine=request.param)
         monkeypatch.chdir(datapath("io", "data"))
         monkeypatch.setattr(pd, 'ExcelFile', func)
 
     def test_excel_passes_na(self, read_ext):
 
-        excel = ExcelFile('test4' + read_ext)
+        excel = pd.ExcelFile('test4' + read_ext)
path: pandas/tests/io/excel/test_readers.py
text: @author:
We were monkeypatching `pd.ExcelFile` but used `ExcelFile` imported directly in tests which was causing this to run exclusive of the specified engine. Figured easiest just to stick with the pd namespace

diff_hunk: @@ -736,39 +735,44 @@ class TestExcelFileRead:
         pytest.param(None, marks=pytest.mark.skipif(
             not td.safe_import("xlrd"), reason="no xlrd")),
     ])
-    def cd_and_set_engine(self, request, datapath, monkeypatch):
+    def cd_and_set_engine(self, request, datapath, monkeypatch, read_ext):
         """
         Change directory and set engine for ExcelFile objects.
         """
+        if request.param == 'openpyxl' and read_ext == '.xls':
+            pytest.skip()
+
         func = partial(pd.ExcelFile, engine=request.param)
         monkeypatch.chdir(datapath("io", "data"))
         monkeypatch.setattr(pd, 'ExcelFile', func)
 
     def test_excel_passes_na(self, read_ext):
 
-        excel = ExcelFile('test4' + read_ext)
+        excel = pd.ExcelFile('test4' + read_ext)
 
         parsed = pd.read_excel(excel, 'Sheet1', keep_default_na=False,
                                na_values=['apple'])
         expected = DataFrame([['NA'], [1], ['NA'], [np.nan], ['rabbit']],
                              columns=['Test'])
         tm.assert_frame_equal(parsed, expected)
 
+        excel = pd.ExcelFile('test4' + read_ext)
path: pandas/tests/io/excel/test_readers.py
text: @author:
openpyxl seems to exhaust a `pd.ExcelFile` after reading whereas xlrd does not. I don't think we really have a preference here and I can't imagine that the xlrd behavior is necessarily required so just created a new pd.ExcelFile after each use

diff_hunk: @@ -778,7 +782,7 @@ def test_excel_passes_na(self, read_ext):
     @pytest.mark.parametrize('arg', ['sheet', 'sheetname', 'parse_cols'])
     def test_unexpected_kwargs_raises(self, read_ext, arg):
         # gh-17964
-        excel = ExcelFile('test1' + read_ext)
+        excel = pd.ExcelFile('test1' + read_ext)
path: pandas/tests/io/excel/test_readers.py
text: @user1:
do we normally need to close the file? use in a context manager?

@author:
It looks like it depends on what type of object ExcelFile is interacting with but ultimately converted usage to a context manager to be safe