Test pytables refactor #28746

tolaa001 · 2019-10-02T10:05:57Z

precursor to TST: split out test_pytables.py to sub-module of tests #18498 and continuation of precursor to Split out test_pytables.py to sub-module of tests #28715
No new tests added
black pandas ran successfully
This PR is the next step reference in precursor to Split out test_pytables.py to sub-module of tests #28715.

@simonjayhawkins and @jreback as promised, this PR contains the following new and EXCITING changes:

Created a contest.py in the pytables sub directory containing all the shared fixtures
Created common.py which contains all the functions and variables used by the test classes such as tables, safe_remove, safe_close, create_tempfile, ensure_clean_path, ensure_clean_store, _maybe_remove
Created separate modules for each of the test classes i.e. test_timezones, test_hdf_store, test_hdf_complex_values
As a result of creating the common.py, I have update the ensure_clean_path import to the test_compat module to ensure safe deletion of the test_pytables module.

There has been no amendments to the body of the code. The next PR will involve focusing on removing the class in test_timezone and making it strictly functional. Followed by me doing this to the other 2 new modules i.e. test_hdf_store and test_hdf_complex_values.

Once the code base is strictly functional, I can start working on more fun refactoring of the functions :)

…s share

…nt imports

…ated test_hdf_store.py

…dules will be functional

jreback

lgtm. just the 2 changes on moving the tests in already separately files. all other comments are for future PRs. ping on green.

jreback · 2019-10-02T11:33:44Z

pandas/tests/io/pytables/common.py

+            pass
+
+
+def safe_close(store):


in a future PR, we want to have doc-strings & typing for these

type errors in tests directories are not reported by mypy

pandas/setup.cfg

Lines 133 to 134 in 75c9783

[mypy-pandas.conftest,pandas.tests.*]

ignore_errors=True

so should typing be added? does help grok functions but equally if not correct (since not checked) could be confusing.

pandas/tests/io/pytables/test_complex.py

pandas/tests/io/pytables/test_timezones.py

jreback · 2019-10-02T11:35:11Z

pandas/tests/io/pytables/test_timezones.py

+import pandas as pd
+from pandas import DataFrame, DatetimeIndex, Series, Timestamp, date_range
+from pandas.tests.io.pytables.common import (
+    _maybe_remove,


in a future PR we can likely completely remove _maybe_remove

pep8speaks · 2019-10-02T14:24:09Z

Hello @tolaa001! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-04 10:09:29 UTC

…actor

jreback

looking good. one more change then i think good to do. ping on green.

jreback · 2019-10-02T16:39:43Z

pandas/tests/io/pytables/test_complex.py

+
+from pandas.io.pytables import read_hdf
+
+# TODO:


can you move these tables imports and mods (lines 15-22) from these individual files and put in common instead.

that way we only do these one and skip appropriately.

@jreback I think pytest.importorskip("tables") should be in each module and common.py should be pytest agnostic.

but that does mean duplication of parameter setting unless we add that to the module scoped fixture.

…actor

jreback · 2019-10-03T10:59:16Z

pandas/tests/series/test_operators.py

@@ -94,24 +94,25 @@ def test_logical_operators_int_dtype_with_int_scalar(self):
    def test_logical_operators_int_dtype_with_float(self):
        # GH#9016: support bitwise op for integer types
        s_0123 = Series(range(4), dtype="int64")
-
-        with pytest.raises(TypeError):
+        msg = r"cannot compare a dtyped \[int64\] array with a scalar of type \[float\]"


why are you changing things here?

apologies, this shouldn't have been a change here as I was working on another branch working on the bare pytest_raises issue.

jreback · 2019-10-03T17:30:44Z

@tolaa001 you have some failing checks, if you can fixup we should merge this soon to prevent conflicts.

…actor

tolaa001 · 2019-10-03T18:59:48Z

@tolaa001 you have some failing checks, if you can fixup we should merge this soon to prevent conflicts.

@jreback all green now and reverted my accidental changes to test_operators.py. Thank you again for reviewing this

jreback · 2019-10-03T19:06:10Z

pandas/tests/io/pytables/test_complex.py

+
+from pandas.io.pytables import read_hdf
+
+tables = pytest.importorskip("tables")


right can you move these to common.py (the tables.parameters.*) that are repeated across files.

you can also move the pytest.importorskip to common.py as well

But this would mean having to import pytest in your common module, is this something you'd be ok with?

@jreback would this be acceptable?

diff --git a/pandas/tests/io/pytables/conftest.py b/pandas/tests/io/pytables/conftest.py index 6164f5d07..db00b1e33 100644 --- a/pandas/tests/io/pytables/conftest.py +++ b/pandas/tests/io/pytables/conftest.py @@ -15,3 +15,14 @@ def setup_mode(): tm.reset_testing_mode() yield tm.set_testing_mode() + + +@pytest.fixture(scope="module", autouse=True) +def set_parameters(): + """ + Set pytables parameters so we don't have file sharing. + """ + tables = pytest.importorskip("tables") + tables.parameters.MAX_NUMEXPR_THREADS = 1 + tables.parameters.MAX_BLOSC_THREADS = 1 + tables.parameters.MAX_THREADS = 1 diff --git a/pandas/tests/io/pytables/test_complex.py b/pandas/tests/io/pytables/test_complex.py index 96c4a3042..37739b1fd 100644 --- a/pandas/tests/io/pytables/test_complex.py +++ b/pandas/tests/io/pytables/test_complex.py @@ -14,10 +14,6 @@ from pandas.util.testing import assert_frame_equal from pandas.io.pytables import read_hdf tables = pytest.importorskip("tables") -# set these parameters so we don't have file sharing -tables.parameters.MAX_NUMEXPR_THREADS = 1 -tables.parameters.MAX_BLOSC_THREADS = 1 -tables.parameters.MAX_THREADS = 1 # GH10447 diff --git a/pandas/tests/io/pytables/test_store.py b/pandas/tests/io/pytables/test_store.py index 97fd89903..e31cb0cf6 100644 --- a/pandas/tests/io/pytables/test_store.py +++ b/pandas/tests/io/pytables/test_store.py @@ -56,10 +56,6 @@ from pandas.io.pytables import TableIterator # noqa: E402 isort:skip tables = pytest.importorskip("tables") -# set these parameters so we don't have file sharing -tables.parameters.MAX_NUMEXPR_THREADS = 1 -tables.parameters.MAX_BLOSC_THREADS = 1 -tables.parameters.MAX_THREADS = 1 _default_compressor = "blosc" diff --git a/pandas/tests/io/pytables/test_timezones.py b/pandas/tests/io/pytables/test_timezones.py index 3e91c6381..b18fdc7b6 100644 --- a/pandas/tests/io/pytables/test_timezones.py +++ b/pandas/tests/io/pytables/test_timezones.py @@ -16,10 +16,6 @@ import pandas.util.testing as tm from pandas.util.testing import assert_frame_equal, set_timezone tables = pytest.importorskip("tables") -# set these parameters so we don't have file sharing -tables.parameters.MAX_NUMEXPR_THREADS = 1 -tables.parameters.MAX_BLOSC_THREADS = 1 -tables.parameters.MAX_THREADS = 1 def _compare_with_tz(a, b):

the other alternative is to monkeypatch..

@pytest.fixture(autouse=True) def set_parameters(monkeypatch): """ Set pytables parameters so we don't have file sharing. """ monkeypatch.setattr("tables.parameters.MAX_NUMEXPR_THREADS", 1, raising=False) monkeypatch.setattr("tables.parameters.MAX_BLOSC_THREADS", 1, raising=False) monkeypatch.setattr("tables.parameters.MAX_THREADS", 1, raising=False)

but this needs to be a function scoped fixture, see pytest-dev/pytest#363

in theory it would be better as a fixture but just trying to avoid code duplication

this was always somewhat leaky because it globally patches

it could be nicer but can do later

ok. but IMO common.py should not have pytest imported.

also, some of the functions in common could be fixturized (and maybe eventually remove common.py)

i'm not convinced from the pytest docs that pytest.importorskip should be used outside the module, but it seems to work.

but we need to merge ASAP to avoid merge conflicts, so i'll not block this further.

so what is the verdict on tables for this PR?

so what is the verdict on tables for this PR?

see #28746 (comment)

jreback · 2019-10-05T22:35:25Z

thanks @tolaa001 very nice.

love to have the followups as discussed above.

tolaa001 added 11 commits October 2, 2019 09:26

added a conftest.py to pytables subdir

b7450c7

added common.py file to migrate the functions which all 3 test classe…

480184d

…s share

moved TestTimezone class to test_timezones.py and relevant imports

31d6836

moved HDFComplexvalues class to test_hdf_complex_values.py and releva…

e3242f4

…nt imports

updated common.py, updated imports in test_hdf_complex_values and cre…

1985c96

…ated test_hdf_store.py

removed tables variable as not required for Timezone class

e93f3df

cleaned up unused imports

80d6aaf

updated import for ensure_clean_path to reference common.py instead

7f0842b

deleted test_pytables.py after splitting contents across 4 python files

882ba8e

created test_hdf_store module to move class TestHDFStore

54483fb

Ran black formatting

cd6a2c9

simonjayhawkins added Clean IO HDF5 read_hdf, HDFStore Testing pandas testing functions or related to the test suite labels Oct 2, 2019

tolaa001 added 4 commits October 2, 2019 11:45

amended scope for setup_mode to module as the future state of test mo…

eb14b23

…dules will be functional

running isort to import in the right order

7b640fc

moving tables from common.py to the modules requiring this

44c56f1

renaming test files to make them sexier

595670e

jreback requested changes Oct 2, 2019

View reviewed changes

jreback added this to the 1.0 milestone Oct 2, 2019

tolaa001 added 3 commits October 2, 2019 14:38

copied pytest.importorskip to timezone

515825a

removed test_timezone class and kept module functional

6106a24

removed test_hdfcomplexvalues class and kept module functional

8f540a7

tolaa001 added 2 commits October 2, 2019 16:08

convert test_store back to class to make git diff easier for review

cfdd437

Merge remote-tracking branch 'upstream/master' into test_pytables_ref…

6375977

…actor

tolaa001 force-pushed the test_pytables_refactor branch from 6c71670 to 6375977 Compare October 2, 2019 15:20

moved xfail_non_writeable into _test_decorators

aa97d78

tolaa001 requested a review from jreback October 2, 2019 16:14

jreback requested changes Oct 2, 2019

View reviewed changes

tolaa001 added 6 commits October 2, 2019 18:09

removed numpy from safe import

9931352

importing numpy instead of using safe import

27d62e4

Merge remote-tracking branch 'upstream/master' into test_pytables_ref…

2e83dfd

…actor

removing unused imports

eeac6ce

added tables to xfail decorator

78c710b

changed import for decorator

a38bc5e

jreback reviewed Oct 3, 2019

View reviewed changes

tolaa001 added 2 commits October 3, 2019 19:22

reverted accidentally changes to test_operator.py

6d97676

Merge remote-tracking branch 'upstream/master' into test_pytables_ref…

64ec41d

…actor

jreback reviewed Oct 3, 2019

View reviewed changes

tolaa001 added 2 commits October 4, 2019 11:06

moved tables to common.py

49e9d31

fixed import

e5cecce

tolaa001 requested a review from jreback October 4, 2019 10:47

jreback approved these changes Oct 5, 2019

View reviewed changes

jreback merged commit 2a429af into pandas-dev:master Oct 5, 2019

simonjayhawkins mentioned this pull request Oct 11, 2019

TYPING: lockdown test modules passing mypy #28914

Merged

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Test pytables refactor (pandas-dev#28746)

2c27802

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Test pytables refactor (pandas-dev#28746)

3e6ca80

bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020

Test pytables refactor (pandas-dev#28746)

17e6cc3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test pytables refactor #28746

Test pytables refactor #28746

tolaa001 commented Oct 2, 2019

jreback left a comment

jreback Oct 2, 2019

simonjayhawkins Oct 2, 2019

jreback Oct 2, 2019

pep8speaks commented Oct 2, 2019 •

edited

Loading

jreback left a comment

jreback Oct 2, 2019

simonjayhawkins Oct 2, 2019

jreback Oct 3, 2019

tolaa001 Oct 3, 2019

jreback commented Oct 3, 2019

tolaa001 commented Oct 3, 2019

jreback Oct 3, 2019

tolaa001 Oct 3, 2019

simonjayhawkins Oct 3, 2019

simonjayhawkins Oct 3, 2019

jreback Oct 3, 2019

simonjayhawkins Oct 3, 2019

tolaa001 Oct 3, 2019

simonjayhawkins Oct 3, 2019

jreback commented Oct 5, 2019


		from pandas.io.pytables import read_hdf

		tables = pytest.importorskip("tables")

Test pytables refactor #28746

Test pytables refactor #28746

Conversation

tolaa001 commented Oct 2, 2019

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Oct 2, 2019 • edited Loading

Comment last updated at 2019-10-04 10:09:29 UTC

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 3, 2019

tolaa001 commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 5, 2019

pep8speaks commented Oct 2, 2019 •

edited

Loading