Fixturize JSON tests #31191

WillAyd · 2020-01-22T01:13:14Z

Ultimately looking to split this module up into something like:

test_serializer.py
test_deserializer.py
test_roundtrip.py
test_orient.py

TBD on those being the actual modules, but for now starting with some parametrization

jbrockmendel · 2020-01-22T01:57:42Z

pandas/tests/io/json/test_pandas.py

@@ -681,10 +662,10 @@ def test_series_roundtrip_empty(self, orient, numpy):
        tm.assert_series_equal(result, expected)

    @pytest.mark.parametrize("numpy", [True, False])
-    def test_series_roundtrip_timeseries(self, orient, numpy):
-        data = self.ts.to_json(orient=orient)
+    def test_series_roundtrip_timeseries(self, orient, numpy, datetime_series):


this looks really similar to test_series_roundtrip_empty and test_series_roundtrip_object. would it make sense to something like any_series fixture for this?

Yea I think a great idea. I’m thinking worth doing after breaking out and setting up test_roundtrip.py with other parametrization unless a blocker here

sounds good. This LGTM

simonjayhawkins

Thanks @WillAyd a few comments otherwise lgtm

simonjayhawkins · 2020-01-22T13:44:13Z

pandas/tests/io/json/test_pandas.py

@@ -474,12 +455,12 @@ def test_v12_compat(self):
        df["modified"] = df["date"]
        df.iloc[1, df.columns.get_loc("modified")] = pd.NaT

-        v12_json = os.path.join(self.dirpath, "tsframe_v012.json")
+        v12_json = os.path.join("tsframe_v012.json")


you could just use the filename string directly in the read_json call below?

simonjayhawkins · 2020-01-22T13:44:49Z

pandas/tests/io/json/test_pandas.py

        df_unser = pd.read_json(v12_json)
        tm.assert_frame_equal(df, df_unser)

        df_iso = df.drop(["modified"], axis=1)
-        v12_iso_json = os.path.join(self.dirpath, "tsframe_iso_v012.json")
+        v12_iso_json = os.path.join("tsframe_iso_v012.json")


simonjayhawkins · 2020-01-22T13:50:09Z

pandas/tests/io/json/test_pandas.py

        result = pd.read_json(data, typ="series", orient=orient, numpy=numpy)
-        expected = self.series.copy()
+        expected = string_series.copy()


strictly don't need to copy as fixture creates new object for each invocation of test

simonjayhawkins · 2020-01-22T13:51:15Z

pandas/tests/io/json/test_pandas.py

        result = pd.read_json(
            data, typ="series", orient=orient, numpy=numpy, dtype=dtype
        )
-        expected = self.objSeries.copy()
+        expected = object_series.copy()


simonjayhawkins · 2020-01-22T13:51:43Z

pandas/tests/io/json/test_pandas.py

        result = pd.read_json(data, typ="series", orient=orient, numpy=numpy)
-        expected = self.empty_series.copy()
+        expected = empty_series.copy()


simonjayhawkins · 2020-01-22T13:52:53Z

pandas/tests/io/json/test_pandas.py

        result = pd.read_json(data, typ="series", orient=orient, numpy=numpy)
-        expected = self.ts.copy()
+        expected = datetime_series.copy()


simonjayhawkins · 2020-01-22T13:59:56Z

pandas/tests/resample/conftest.py

@@ -126,7 +126,7 @@ def series(index, _series_name, _static_values):


 @pytest.fixture
-def empty_series(series):
+def empty_series_dti(series):


why change this? doesn't the series type depends on the _index_factory fixture.

Trying to avoid naming conflicts with the top level empty_series fixture, which I think is more general; would they not conflict if named the same?

see #24886, @jbrockmendel removed empty series fixture since it is a singleton and inlined instead. adding a top-level empty_series fixture is inconsistent with the series tests.

The fixture here is a composable fixture and more generic and IMO should not be renamed.

Does the fixture added in this PR need to be in the top level conftest or could it be just added to pandas/tests/io/json/test_pandas.py as a module level fixture?

simonjayhawkins · 2020-01-22T14:03:48Z

pandas/tests/resample/test_base.py

@@ -93,13 +93,13 @@ def test_raises_on_non_datetimelike_index():

 @all_ts
 @pytest.mark.parametrize("freq", ["M", "D", "H"])
-def test_resample_empty_series(freq, empty_series, resample_method):
+def test_resample_empty_series(freq, empty_series_dti, resample_method):


IIUC the all_ts decorator is running this test for date_range, period_range and timedelta_range, not just dti

jreback

we had basically these same fixtures, then removed them, now are adding them back (in pandas/conftest.py)

can you see if these already exist in other conftests.py

we also need a script to deduplicate fixtures (and check this)

jreback · 2020-01-23T14:17:12Z

pandas/tests/io/json/test_pandas.py

-    def setup(self, datapath):
-        self.dirpath = datapath("io", "json", "data")
+    @pytest.fixture(autouse=True)
+    def setup(self, datapath, monkeypatch):


what? why would you need to chdir?

We use this same idiom in the excel file tests to read in files directly from where they are stored. Can alternately use datapath directly in the tests at the cost of more code duplication

this is a really strange pattern, would prefer to clean it up

i find this a really strange pattern which is not standard and likely not even needed

pls fix

WillAyd · 2020-01-23T19:40:34Z

we had basically these same fixtures, then removed them, now are adding them back (in pandas/conftest.py)

With the exception of empty_series this isn't adding anything but instead moving from pandas/tests/series/conftest.py to the higher level conftest for re-use. Can alternately duplicate definitions in JSON conftest if we don't like them at the high level, but note that there are quite a few other tests areas that could reuse these rather than calling tm.make* functions directly

jreback · 2020-01-24T00:49:09Z

we had basically these same fixtures, then removed them, now are adding them back (in pandas/conftest.py)

With the exception of empty_series this isn't adding anything but instead moving from pandas/tests/series/conftest.py to the higher level conftest for re-use. Can alternately duplicate definitions in JSON conftest if we don't like them at the high level, but note that there are quite a few other tests areas that could reuse these rather than calling tm.make* functions directly

i think you are not seeing the big picture here. I am all for making fixtures top-level and not speciying them at a lower level. However, we have had a number of PRs moving them to lower levels, and then others moving them up.

Before we merge this we need a check to avoid duplication of fixtures.

WillAyd · 2020-01-24T00:53:57Z

Ah OK I get what you are asking. So running pytest pandas --fixtures and searching for these would manually confirm they aren't duplicated. I'll check with the pytest folks to see if they offer anything more robust

jbrockmendel · 2020-02-10T17:16:23Z

needs rebase

jbrockmendel · 2020-02-12T02:21:57Z

pandas/tests/io/json/test_pandas.py

@@ -648,7 +650,7 @@ def test_series_roundtrip_empty(self, orient, numpy, empty_series):
        data = empty_series.to_json(orient=orient)
        result = pd.read_json(data, typ="series", orient=orient, numpy=numpy)

-        # TODO: see what causes inconsistency


did this get resolved?

pandas/conftest.py

jbrockmendel · 2020-02-12T02:24:17Z

unused import, otherwise LGTM

jbrockmendel · 2020-02-19T20:38:02Z

needs rebase, ping on green

jbrockmendel · 2020-02-21T02:51:56Z

cc @jreback im about ready to merge this, any objection?

jreback

this needs some work

if u fixturize then remove the setup

jreback · 2020-02-21T04:41:21Z

pandas/tests/io/json/test_pandas.py

-    def setup(self, datapath):
-        self.dirpath = datapath("io", "json", "data")
+    @pytest.fixture(autouse=True)
+    def setup(self, datapath, monkeypatch):


i find this a really strange pattern which is not standard and likely not even needed

pls fix

jreback · 2020-02-21T04:42:04Z

pandas/tests/io/json/test_pandas.py

-        self.dirpath = datapath("io", "json", "data")
+    @pytest.fixture(autouse=True)
+    def setup(self, datapath, monkeypatch):
+        monkeypatch.chdir(datapath("io", "json", "data"))


why is there even a setup left? this is very messy

I can't get rid of the setup just yet because would still need to take care of all of the frame fixtures (this PR only has done Series)

But yea goal would be to get rid of it

jreback · 2020-03-15T00:59:26Z

can you merge master and see if you can avoid the monkeypatch

simonjayhawkins

I'd rather empty_series_dti wasn't named that way. see #31191 (comment) and #31191 (comment)

Having different fixtures with the same name shouldn't cause conflicts if the directory structure is properly organised and in this case I see no reason to rename it.

If you are renaming can you avoid the use of dti? maybe empty_series_datetimelike.

WillAyd · 2020-03-18T22:57:31Z

Having different fixtures with the same name shouldn't cause conflicts if the directory structure is properly organised and in this case I see no reason to rename it.

I don't think we are worried about this causing errors as much as being confusing; I don't know that we want to override fixtures in different packages, and Jeff was asking previously for some way to check that we explicitly don't do this (unless I misunderstood)

If you are renaming can you avoid the use of dti? maybe empty_series_datetimelike.

These are all empty series with datetimelike-typed indices, which is why I went for dti. I personally would misread series_datetimelike as referring to the values not the indices; @jbrockmendel since you have the most expertise in that area - can you offer an opinion?

jbrockmendel · 2020-03-18T23:30:55Z

I personally would misread series_datetimelike as referring to the values not the indices

I would be at risk of this too.

This naming convention problem is not easy and is not unique to this PR, should not hold it up.

AFAICT there are three options for getting informative names:

long names e.g. "datetime64_series_with_interval_index"
adopt some predictable conventions to shorten these e.g. "dt64ser_ii"
live with the not-really-that-bad status quo

jreback · 2020-03-19T00:40:25Z

lgtm. am a little concerted about changing to use empty_series_dti, but agree we need to fix this; so ok for now (maybe open a followup issue for discussion).

any more @simonjayhawkins or @jbrockmendel

jbrockmendel · 2020-03-19T03:18:03Z

im a happy camper

simonjayhawkins · 2020-03-19T09:33:04Z

Thanks @WillAyd

WillAyd added 3 commits January 21, 2020 17:04

Initial fixturization

730ed79

parametrized series objects

1a96b4f

Black

dfebc6b

WillAyd added Testing pandas testing functions or related to the test suite IO JSON read_json, to_json, json_normalize labels Jan 22, 2020

jbrockmendel reviewed Jan 22, 2020

View reviewed changes

simonjayhawkins requested changes Jan 22, 2020

View reviewed changes

simonjayhawkins added this to the 1.1 milestone Jan 22, 2020

jreback requested changes Jan 23, 2020

View reviewed changes

WillAyd added 2 commits February 6, 2020 16:33

Merge remote-tracking branch 'upstream/master' into json-test-cleanup

e793582

simonjayhawkins feedback

d507eb4

WillAyd added 2 commits February 11, 2020 17:26

Merge remote-tracking branch 'upstream/master' into json-test-cleanup

224dd5a

fix ci failures

0056824

jbrockmendel reviewed Feb 12, 2020

View reviewed changes

pandas/conftest.py Show resolved Hide resolved

WillAyd added 2 commits February 12, 2020 08:34

Merge remote-tracking branch 'upstream/master' into json-test-cleanup

bb808df

flake8

9f57295

Merge remote-tracking branch 'upstream/master' into json-test-cleanup

65de92a

jreback requested changes Feb 21, 2020

View reviewed changes

WillAyd added 2 commits March 16, 2020 19:09

Merge remote-tracking branch 'upstream/master' into json-test-cleanup

2b78450

removed monkeypatch

4e80178

simonjayhawkins requested changes Mar 17, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into json-test-cleanup

083bfdb

WillAyd added 2 commits March 18, 2020 16:32

reverted test_base change

e60430b

black

dc2d93e

jreback approved these changes Mar 19, 2020

View reviewed changes

simonjayhawkins merged commit f6099bf into pandas-dev:master Mar 19, 2020

SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020

Fixturize JSON tests (pandas-dev#31191)

45ae04d

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 23, 2020

Fixturize JSON tests (pandas-dev#31191)

a7e0f14

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 25, 2020

Fixturize JSON tests (pandas-dev#31191)

92cf475

WillAyd mentioned this pull request Mar 30, 2020

More Json parametrize #33148

Merged

WillAyd deleted the json-test-cleanup branch April 12, 2023 20:17

Fixturize JSON tests #31191

Fixturize JSON tests #31191

Conversation

WillAyd commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins 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

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 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

WillAyd commented Jan 23, 2020

jreback commented Jan 24, 2020

WillAyd commented Jan 24, 2020

jbrockmendel commented Feb 10, 2020

Choose a reason for hiding this comment

jbrockmendel commented Feb 12, 2020

jbrockmendel commented Feb 19, 2020

jbrockmendel commented Feb 21, 2020

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 Mar 15, 2020

simonjayhawkins left a comment • edited Loading

Choose a reason for hiding this comment

WillAyd commented Mar 18, 2020

jbrockmendel commented Mar 18, 2020

jreback commented Mar 19, 2020

jbrockmendel commented Mar 19, 2020

simonjayhawkins commented Mar 19, 2020

simonjayhawkins left a comment •

edited

Loading