TST: [ArrowStringArray] more parameterised testing - part 2

[simonjayhawkins]

only a few tests updated in this PR to ensure this approach is accepted.

if we try to add directly to the parametrization of the existing tests we get ImportError on environments without pyarrow installed. #40679 (comment)

[simonjayhawkins]

I've added this to force the registration of the arrow_string dtype. running some tests individually sometimes fails. will be removed in #39908

[jreback]

you need to handle the import error here and xfail

[simonjayhawkins]

can you elaborate or I can just remove.

[jreback]

this would raise an import error right on some builds? if so simply xfail if this errors and the tests would also xfail

that way u can use this fixture w/o worrying too much

[simonjayhawkins]

The tests are skipped with the fixture. It's passing tests that may fail if the arrow_string dtype is not registered, it's not part of the public api yet. and won't be since we are going to use the parameterized dtype in #39908 instead.

[simonjayhawkins]

see also #39908 (comment)

[simonjayhawkins]

this would raise an import error right on some builds?

no. importing the ArrowStringDtype is safe.

[jreback]

i c, ok then

[jreback]

may want to consider using a shared function as the impl to avoid repeating things. though maybe not worth it (e.g. just copy in the cases where this is needed if there are not that many)

[jreback]

do we have a fixture for errors? if not can u create one

[jorisvandenbossche]

Alternatively, could you parametrize the test above, but have the dtype be a separate param, so it can be skipped for arrow_string?

Something like

    @pytest.mark.parametrize(
        "data, dtype",
        [
            (["x", "y", "z"], "string"),
            pytest.param((["x", "y", "z"], "string_arrow"), mark=..skip...),
            (["x", "y", "z"], "category"),
            (3 * [Timestamp("2020-01-01", tz="UTC")], None),
            (3 * [Interval(0, 1)], None),
        ],
    )

and then inside the test create the dataframe (df = DataFrame(Series(data, dtype=dtype))).

That could also avoid having to duplicate the full test body.

[jorisvandenbossche]

(although for the other tests where pd.array is used in the parametrization, this might get a bit more annoying to do it in this way ..)

[simonjayhawkins]

do we have a fixture for errors? if not can u create one

we have no fixture yet in conftest.py (common arguments section) for errors

we have 4 tests parameterized on errors...

test_astype_ignores_errors_for_extension_dtypes in pandas/tests/frame/methods/test_astype.py and pandas/tests/series/methods/test_astype.py parameterised on ["raise", "ignore"]

test_custom_dateparsing_error in pandas/tests/io/test_sql.py parameterised on ["ignore", "raise", "coerce"]

and

test_coerce_outside_ns_bounds in pandas/tests/tslibs/test_array_to_datetime.py parameterised on ["coerce", "raise"]

will be pushing a commit shortly with @jorisvandenbossche suggested changes to avoid duplication so doing anything with errors parameterisation is now outside the scope of this PR.

I don't think worth opening a separate PR for that yet, especially considering the differences in the parameterisation

[simonjayhawkins]

(although for the other tests where pd.array is used in the parametrization, this might get a bit more annoying to do it in this way ..)

the other two tests have simpler code so duplication is probably better than complex setup

test_astype_ignores_errors_for_extension_dtypes is already duplicated in frame and series methods modules so makes more sense to avoid adding more duplication there. have updated with suggestion.

[jorisvandenbossche]

The failure looks like a flaky ResourceWarning related one.

[simonjayhawkins]

yes. but I restarted anyway.