Revert "REF: remove unnecessary na_value fixture"

[jorisvandenbossche]

This reverts one of the four commits of #54719.

While this was certainly a logical change to make (it's superfluous for our tests), it can give problems for downstream EAs (we got several failures in the geopandas' CI with pandas nightly), if they use None as their missing value.

The problem is that pandas didn't really support to properly set None as the ExtensionDtype.na_value (see eg #44602), and so we relied on the fixture to use the correct value for the tests while using the default of NaN for the attribute.

[jbrockmendel]

The problem is that pandas didn't really support to properly set None as the ExtensionDtype.na_value (see eg #44602), and so we relied on the fixture to use the correct value for the tests while using the default of NaN for the attribute.

Wouldn't actually addressing this be the correct solution?

[jorisvandenbossche]

Yes, certainly, and we have a two year old open issue about it.

But nevertheless in the meantime it would be easier for downstream EAs that use None to keep the tests as they were

[jbrockmendel]

But nevertheless in the meantime

Is this time-sensitive? Or just "before 2.2"? If the latter we should take some time to try to address the underlying issue.

[phofl]

Let's deprecate this then so that EA authors have time to adjust but we can clean it up at some point

[jorisvandenbossche]

Is this time-sensitive? Or just "before 2.2"? If the latter we should take some time to try to address the underlying issue.

It's "time sensitive" in the sense that it is annoying that we have a bunch of failing tests on our CI build with nightly pandas.

And I also don't see the issue with just merging this. We can easily change it back at some point when the issue is fixed (or tested, as it might already be fixed), although even then, we might want to wait a while with changing the test, as downstream EAs still test against multiple pandas versions typically.

[jorisvandenbossche]

Let's deprecate this then

Do you mean to deprecate the pytest fixture? (not fully sure how that would work)

[jbrockmendel]

I commented on #44602; I expect the geopandas problem is subtly different

[jorisvandenbossche]

Yes, the problem for geopandas is that it supports multiple pandas versions, and this was only apparently fixed in a recent version.

[jbrockmendel]

Yes, the problem for geopandas is that it supports multiple pandas versions, and this was only apparently fixed in a recent version.

Hmm i dont think we have a policy on the extension backward/forward compatibility of the extension tests. Tests in general I wouldn't expect to work with anything but the matching version.

[github-actions]

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

[jorisvandenbossche]

Tests in general I wouldn't expect to work with anything but the matching version.

Of course the tests only work in the matching version, and downstream EA implementation import those tests from pandas, and thus in practice only test with matching versions.
But the issue here is not supporting the test across versions, but supporting the dtype na_value (i.e. #44602) across pandas version.

This PR is a very simple change, that makes the life easier for downstream testing in geopandas. So unless there is actual strong objection to do so, I am planning to merge this.

[jbrockmendel]

But the issue here is not supporting the test across versions, but supporting the dtype na_value (i.e. #44602) across pandas version.

As I commented in #44602, that appears to be working without this kludge. IIUC you said the geopandas use case is working too. If geopandas wants to test against older pandas, they'll need to test against older versions of the extension tests. I don't see any way around that.

As you say this is a small change so I don't care that much. Is there a path to getting rid of this eventually? e.g. keep it around through 2.x and then re-remove it?

[jorisvandenbossche]

As I commented in #44602, that appears to be working without this kludge. IIUC you said the geopandas use case is working too.

This is working for the latest version of pandas, but geopandas cannot yet adopt that because we support a range of pandas versions.