Change cross-pandas-version testing in cudf
#15145
Conversation
galipremsagar
added
3 - Ready for Review
galipremsagar
changed the title
cudf
| @@ -964,6 +977,10 @@ def test_is_decimal_dtype(obj, expect): | |||
| assert types.is_decimal_dtype(obj) == expect | |||
|
|
|||
|
|
|||
| @pytest.mark.skipif( | |||
There was a problem hiding this comment.
non-blocking: If possible, would be great to prefer using xfail(..., raises=AssertionError) over skipif so these tests still have an opportunity to exercise cudf APIs as a smoketest
There was a problem hiding this comment.
Agreed. If it doesn't pass, then use xfail. Only use skipif if you have to avoid execution entirely, e.g. to avoid a segfault.
There was a problem hiding this comment.
I don't think using xfail instead of skipif is a good option. Here's why: Each version of pandas behaves differently and we have enabled strict xfail globally. If we start xfailing, we will end up versioning those xfails's heavily like prior to this PR adding more maintenance burden.
There was a problem hiding this comment.
This strategy would be good to explain in docs/cudf/source/developer_guide/testing.md.
There was a problem hiding this comment.
Sorry for not responding earlier, I'm just catching up on a bunch of notifications today. Yeah, I think skipif is unfortunately probably the better option here (full disclosure I think I also originally proposed this setup) for precisely the reason that Prem mentioned. We've definitely had edge cases before where we would do things like "fail if Y > pandas > X" because the xfail behavior wasn't even necessarily monotonic in versions (i.e. it also wouldn't show up in pandas < X). The current approach avoids that complexity, and also avoids degrading the developer experience if a dev locally happens to have one of the failing versions of pandas installed and thinks they have a problematic test. With the current strategy it's run everything if you're on exactly the right pandas version, otherwise skip all the tests that failed on any prior pandas version within the nominally supported range.
There was a problem hiding this comment.
Generally looks good. I support @mroeschke's proposal to use more xfail and less skipif.
galipremsagar
added
5 - Ready to Merge
|
/merge |
This was required by rapidsai#15109, but removed by the changes in rapidsai#15145 and the merge order was such that they weren't tested against each other.
This was required by #15109, but removed by the changes in #15145 and the merge order was such that they weren't tested against each other. Authors: - Lawrence Mitchell (https://github.com/wence-) Approvers: - Bradley Dice (https://github.com/bdice) - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #15287
Description
This PR removes redundant version checks in a lot of pytests.
Checklist