Remove xpass cases and turn xpass into failures #7267

pllim · 2018-03-08T16:26:53Z

astropy-bot · 2018-03-08T16:26:55Z

Hi there @pllim 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

pllim · 2018-03-08T17:39:01Z

Looks like I opened a can o' worms. Compiling them here so I can see a pattern.

PYTHON_VERSION=3.5 NUMPY_VERSION=1.10 (https://travis-ci.org/astropy/astropy/jobs/350900557)

_ TestUfuncReduceReduceatAccumulate.test_one_argument_ufunc_reduce_accumulate __
[XPASS(strict)] condition: NUMPY_LT_1_13
_________________ TestUfuncOuter.test_one_argument_ufunc_outer _________________
[XPASS(strict)] condition: NUMPY_LT_1_13

PYTHON_VERSION=3.5 NUMPY_VERSION=1.11 (https://travis-ci.org/astropy/astropy/jobs/350900558)

__________________________ test_mad_std_scalar_return __________________________
[XPASS(strict)] 
_ TestUfuncReduceReduceatAccumulate.test_one_argument_ufunc_reduce_accumulate __
[XPASS(strict)] condition: NUMPY_LT_1_13
_________________ TestUfuncOuter.test_one_argument_ufunc_outer _________________
[XPASS(strict)] condition: NUMPY_LT_1_13

Note: test_mad_std_scalar_return was added in #5232 already with xfail in place.

PYTHON_VERSION=3.6 NUMPY_VERSION=1.12 (https://travis-ci.org/astropy/astropy/jobs/350900561)

_ TestUfuncReduceReduceatAccumulate.test_one_argument_ufunc_reduce_accumulate __
[XPASS(strict)] condition: NUMPY_LT_1_13
_________________ TestUfuncOuter.test_one_argument_ufunc_outer _________________
[XPASS(strict)] condition: NUMPY_LT_1_13

keflavich · 2018-03-08T18:16:28Z

astropy/stats/tests/test_funcs.py

+            try:
+                assert not np.isnan(rslt)
+            except AssertionError:
+                pytest.xfail('See #5232')


That PR discussion is really long; maybe add a link to a specific comment that summarizes the expected fail? I don't remember if this particular test is expected to fail only on certain numpy versions or what...

I wasn't involved in that discussion and can't follow it completely (it is very long). Is #5232 (comment) close enough? It does seem weird that this test would only xpass in Numpy 1.11 and nothing else.

...it would probably take me half an hour to figure it out reading back through that. Oh well, let's just leave the comment here - at least it is theoretically possible to reconstruct my mental state at the time...

Okay then. 😉

If that comment is right, this problem will auto-disappear in 3.1, since we'll only support numpy >=1.13

That also suggests, though, this should really be a test on numpy version. I don't really want to hold up this supposedly simple PR, but could you add a comment that might well be resolved in later numpy? Please include something like NUMPY_LT_1_13?? in the comment, so that we'll find it!

bsipocz · 2018-03-08T18:17:45Z

@pllim - Shouldn't we cherry-pick back to 2.0.x, doesn't this affect that branch, too?

pllim · 2018-03-08T18:19:07Z

@bsipocz , do we care about xpass in v2.x? This is more of a "nice to have" than a bug fix. Also, I don't know how painful it is to backport this, given we stripped PY2 support in master now. Feel free to re-milestone as you like.

bsipocz · 2018-03-08T18:37:48Z

@pllim - Oh, well, you have a valid point there, I agree not to backport.

pllim · 2018-03-08T18:55:13Z

Okay. Tests passed. The allowed to fail job that failed is being addressed separately at #7266.

mhvk

Very nice! Only two nitpicks, mostly to avoid future hassle.

mhvk · 2018-03-08T21:02:16Z

astropy/coordinates/tests/test_sky_coord_velocities.py

 def test_transforms_diff(sc):
    # note that arguably this *should* fail for the no-distance cases: 3D
    # information is necessary to truly solve this, hence the xfail
-    trans = sc.transform_to(PrecessedGeocentric(equinox='B1975'))
-    assert isinstance(trans.frame, PrecessedGeocentric)
+    if sc.distance == 1:


If we ever want to test multi-D arrays, this would fail. How about

if not sc.distance.unit.is_equivalent(u.m):

mhvk · 2018-03-08T21:05:04Z

astropy/stats/tests/test_funcs.py

+            try:
+                assert not np.isnan(rslt)
+            except AssertionError:
+                pytest.xfail('See #5232')


If that comment is right, this problem will auto-disappear in 3.1, since we'll only support numpy >=1.13

That also suggests, though, this should really be a test on numpy version. I don't really want to hold up this supposedly simple PR, but could you add a comment that might well be resolved in later numpy? Please include something like NUMPY_LT_1_13?? in the comment, so that we'll find it!

mhvk · 2018-03-08T21:06:00Z

astropy/units/tests/test_quantity_ufuncs.py

@@ -955,6 +954,7 @@ def test_one_argument_ufunc_reduce_accumulate(self):
        with pytest.raises(ValueError):
            np.sin.reduceat(s, i)

+    @pytest.mark.xfail("NUMPY_LT_1_13")


Ah, that's a nice one - the other test method already gives the right failures!

pllim · 2018-03-09T17:15:35Z

@mhvk , if this looks good to you, please approve. Thanks!

mhvk

All OK now. Nice to have this additional robustness. Will merge.

pllim · 2018-03-09T18:28:36Z

Ah, I just noticed this xpass in Numpy dev job. Since it only affects Numpy dev, what is your advice here? This can be a separate PR.

______________________ TestTableFunctions.test_mask_array ______________________
[XPASS(strict)] See https://github.com/astropy/astropy/issues/7214

Remove xpass cases and turn xpass into failures

pllim and others added 2 commits March 8, 2018 11:22

Do not xfail transforms with distance cases

936caea

Enable xfail_strict pytest config by default

be47d33

pllim added testing coordinates no-changelog-entry-needed labels Mar 8, 2018

pllim added this to the v3.0.1 milestone Mar 8, 2018

pllim requested review from eteq and adrn March 8, 2018 16:26

pllim mentioned this pull request Mar 8, 2018

Enable xfail_strict pytest config by default #7194

Closed

Fix xpass failures

4d7e023

pllim added units stats labels Mar 8, 2018

pllim requested review from mhvk and keflavich March 8, 2018 17:55

keflavich reviewed Mar 8, 2018

View reviewed changes

pllim mentioned this pull request Mar 8, 2018

TST: Fix failing remote tests #7266

Merged

mhvk reviewed Mar 8, 2018

View reviewed changes

Address review comments

cc344a4

mhvk approved these changes Mar 9, 2018

View reviewed changes

mhvk merged commit e9fad16 into astropy:master Mar 9, 2018

pllim deleted the coord-xfail branch March 9, 2018 18:28

pllim mentioned this pull request Mar 9, 2018

TST: Fix xpass in test_mask_array for numpy dev #7276

Merged

pllim mentioned this pull request Mar 9, 2018

Revisit workarounds and skipped tests when numpy-1.14.2 is available #7258

Closed

bsipocz pushed a commit that referenced this pull request Mar 12, 2018

Merge pull request #7267 from pllim/coord-xfail

9a484c2

Remove xpass cases and turn xpass into failures

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove xpass cases and turn xpass into failures #7267

Remove xpass cases and turn xpass into failures #7267

pllim commented Mar 8, 2018

astropy-bot bot commented Mar 8, 2018 •

edited

pllim commented Mar 8, 2018 •

edited

keflavich Mar 8, 2018

pllim Mar 8, 2018

keflavich Mar 8, 2018

pllim Mar 8, 2018

mhvk Mar 8, 2018

bsipocz commented Mar 8, 2018

pllim commented Mar 8, 2018

bsipocz commented Mar 8, 2018

pllim commented Mar 8, 2018

mhvk left a comment

mhvk Mar 8, 2018

mhvk Mar 8, 2018

mhvk Mar 8, 2018

pllim commented Mar 9, 2018

mhvk left a comment

pllim commented Mar 9, 2018

Remove xpass cases and turn xpass into failures #7267

Remove xpass cases and turn xpass into failures #7267

Conversation

pllim commented Mar 8, 2018

astropy-bot bot commented Mar 8, 2018 • edited

pllim commented Mar 8, 2018 • edited

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

bsipocz commented Mar 8, 2018

pllim commented Mar 8, 2018

bsipocz commented Mar 8, 2018

pllim commented Mar 8, 2018

mhvk 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

pllim commented Mar 9, 2018

mhvk left a comment

Choose a reason for hiding this comment

pllim commented Mar 9, 2018

astropy-bot bot commented Mar 8, 2018 •

edited

pllim commented Mar 8, 2018 •

edited