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
Conversation
simonjayhawkins
added
Clean
IO Excel
| @@ -67,6 +60,24 @@ def engine(request): | |||
| return request.param | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("kwargs", [dict(), dict(engine=None)]) | |||
There was a problem hiding this comment.
Nice idea but I think the parametrization here is overkill so OK to remove
| @@ -67,6 +60,24 @@ def engine(request): | |||
| return request.param | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("kwargs", [dict(), dict(engine=None)]) | |||
| def test_default_engine(datapath, monkeypatch, kwargs): | |||
| """ | |||
There was a problem hiding this comment.
We don't use docstrings for tests so OK to remove this as well. You can add the issue number as a comment instead
| expected = "xlrd" | ||
|
|
||
| result = pd.ExcelFile("blank.xls", **kwargs) | ||
| assert result.engine == expected |
There was a problem hiding this comment.
This works for ExcelFile - can you think of a way to test this for pd.read_excel as well?
There was a problem hiding this comment.
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"
)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
|
Thanks for the PR. I think this is OK as is in master though, so let's table this for now |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffNoneengine tests with corresponding separate test for default engine value. I also excludedtest_bad_engine_raisesfromTestReadersclass.