Skip to content
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

Jump to bottom

TST: Decoupled more xlrd reading tests from openpyxl #27114

Merged
merged 7 commits into from
Jun 30, 2019
Merged

TST: Decoupled more xlrd reading tests from openpyxl #27114

merged 7 commits into from
Jun 30, 2019

Conversation

WillAyd
Copy link
Member

@WillAyd WillAyd commented Jun 28, 2019

@simonjayhawkins

@WillAyd WillAyd added Testing pandas testing functions or related to the test suite IO Excel read_excel, to_excel labels Jun 28, 2019
WillAyd
WillAyd commented Jun 28, 2019
View reviewed changes
pandas/tests/io/excel/test_readers.py Outdated
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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

pandas/tests/io/excel/test_readers.py Outdated

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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

pandas/tests/io/excel/test_readers.py Outdated
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):
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't really follow the same rules for parametrization as the rest of the items in TestExcelFileReadso figured clearest to remove from class

@WillAyd WillAyd changed the title Decoupled xlrd tests from openpyxl Decoupled more xlrd reading tests from openpyxl Jun 28, 2019
jreback
jreback reviewed Jun 28, 2019
View reviewed changes
pandas/tests/io/excel/test_readers.py Outdated
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we normally need to close the file? use in a context manager?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jreback jreback added this to the 0.25.0 milestone Jun 28, 2019
WillAyd
WillAyd commented Jun 29, 2019
View reviewed changes
pandas/tests/io/excel/test_writers.py
@@ -250,6 +250,7 @@ class and any subclasses, on account of the `autouse=True`
set_option(option_name, prev_engine) # Roll back option change


@td.skip_if_no('xlrd')
Copy link
Member Author

Choose a reason for hiding this comment

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

The writer tests still assume xlrd to be there. Will fix up in a follow up to address proper parametrization here (have focused more on readers given recent PRs)

@simonjayhawkins

@jorisvandenbossche jorisvandenbossche changed the title Decoupled more xlrd reading tests from openpyxl TST: Decoupled more xlrd reading tests from openpyxl Jun 29, 2019
jorisvandenbossche
jorisvandenbossche approved these changes Jun 29, 2019
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Tests pass now locally without having xlrd installed!

@WillAyd WillAyd mentioned this pull request Jun 29, 2019
Merged
@simonjayhawkins
Copy link
Member

This PR results in an additional 2 warnings. although 8 of the same were introduced in #25092.

pandas/tests/io/excel/test_readers.py::TestReaders::test_reader_special_dtypes[openpyxl-.xlsx]
pandas/tests/io/excel/test_readers.py::TestReaders::test_reader_special_dtypes[openpyxl-.xlsm]
pandas/tests/io/excel/test_readers.py::TestReaders::test_reading_all_sheets[openpyxl-.xlsx]
pandas/tests/io/excel/test_readers.py::TestReaders::test_reading_all_sheets[openpyxl-.xlsm]
pandas/tests/io/excel/test_readers.py::TestReaders::test_reading_multiple_specific_sheets[openpyxl-.xlsx]
pandas/tests/io/excel/test_readers.py::TestReaders::test_reading_multiple_specific_sheets[openpyxl-.xlsm]
pandas/tests/io/excel/test_readers.py::TestReaders::test_read_excel_squeeze[openpyxl-.xlsx]
pandas/tests/io/excel/test_readers.py::TestReaders::test_read_excel_squeeze[openpyxl-.xlsm]
pandas/tests/io/excel/test_readers.py::TestExcelFileRead::test_excel_passes_na[openpyxl-.xlsx]
pandas/tests/io/excel/test_readers.py::TestExcelFileRead::test_excel_passes_na[openpyxl-.xlsm]
  C:\Users\simon\Anaconda3\envs\pandas-dev\lib\site-packages\openpyxl\worksheet\_reader.py:296: UserWarning: Unknown extension is not supported and will be removed
    warn(msg)

@jorisvandenbossche
Copy link
Member

@simonjayhawkins that should be solved by #27122

@WillAyd WillAyd merged commit 65ec968 into pandas-dev:master Jun 30, 2019
@WillAyd WillAyd deleted the make-tests-openpyxl-only branch June 30, 2019 03:46
@simonjayhawkins simonjayhawkins mentioned this pull request Jun 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

@simonjayhawkins simonjayhawkins simonjayhawkins approved these changes

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
IO Excel read_excel, to_excel Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.

TST: openpyxl tests fail if xlrd is not installed
4 participants
@WillAyd @simonjayhawkins @jorisvandenbossche @jreback