Skip to content

get rid of the None engine tests and just have a separate test that ensures how that gets bound to a particular engine #26662 (comment) #28749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
+16 −12
Closed

get rid of the None engine tests and just have a separate test that ensures how that gets bound to a particular engine #26662 (comment) #28749

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5da64ba
replace None engine tests for read_excel
ladyyvii Oct 2, 2019
93d84cb
docstring fix
ladyyvii Oct 2, 2019
cdc5f12
addressed PR comments and added skip_if_no decorator
ladyyvii Oct 2, 2019
c42d2f3
added filterwarnings decorator
ladyyvii Oct 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions pandas/tests/io/excel/test_readers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,6 @@ def ignore_xlrd_time_clock_warning():
pytest.mark.filterwarnings("ignore:.*html argument"),
],
),
pytest.param(
None,
marks=[
td.skip_if_no("xlrd"),
pytest.mark.filterwarnings("ignore:.*(tree\\.iter|html argument)"),
],
),
pytest.param("odf", marks=td.skip_if_no("odf")),
]
)
Expand All @@ -67,6 +60,24 @@ def engine(request):
return request.param


@pytest.mark.parametrize("kwargs", [dict(), dict(engine=None)])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea but I think the parametrization here is overkill so OK to remove

def test_default_engine(datapath, monkeypatch, kwargs):
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use docstrings for tests so OK to remove this as well. You can add the issue number as a comment instead

A test for default ExcelFile engine value (which is None)
"""
monkeypatch.chdir(datapath("io", "data"))
expected = "xlrd"

result = pd.ExcelFile("blank.xls", **kwargs)
assert result.engine == expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for ExcelFile - can you think of a way to test this for pd.read_excel as well?

Copy link
Contributor Author

@ladyyvii ladyyvii Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this test does testing implicitly for the default value of the engine parameter (which is None) for pd.read_excel as well, because under the hood pd.read_excel either creates an instance of the ExcelFile class and passes the engine parameter value to it or is provided with an ExcelFile instance through the io parameter:

    if not isinstance(io, ExcelFile):
        io = ExcelFile(io, engine=engine)
    elif engine and engine != io.engine:
        raise ValueError(
            "Engine should not be specified when passing "
            "an ExcelFile - ExcelFile already has the engine set"
        )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I just think that's fragile to always assume that's how this implementation will work - we'd lose coverage and expose ourselves to bugs if this gets refactored

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with you. I'll try to figure out a way to test read_excel as well, but I'm afraid it's not gonna be pretty...



def test_bad_engine_raises():
bad_engine = "foo"
with pytest.raises(ValueError, match="Unknown engine: foo"):
pd.read_excel("", engine=bad_engine)


class TestReaders:
@pytest.fixture(autouse=True)
def cd_and_set_engine(self, engine, datapath, monkeypatch, read_ext):
Expand Down Expand Up @@ -492,11 +503,6 @@ def test_excel_read_buffer(self, read_ext):
actual = pd.read_excel(f, "Sheet1", index_col=0)
tm.assert_frame_equal(expected, actual)

def test_bad_engine_raises(self, read_ext):
bad_engine = "foo"
with pytest.raises(ValueError, match="Unknown engine: foo"):
pd.read_excel("", engine=bad_engine)

@tm.network
def test_read_from_http_url(self, read_ext):
if read_ext == ".ods": # TODO: remove once on master
Expand Down