TST: strict xfail

[rhshadrach]

• Ensure all linting tests pass, see here for how to run them

Part of #38902.

[jbrockmendel]

i think this indicates something that we need to fix. maybe should write out the assertion so that the xfail is meaningful

[rhshadrach]

Thanks @jbrockmendel, I was planning on pinging you and @WillAyd after a bit more digging into this, but you beat me to it :). This was originally changed in #18721 from a comment to the xfail that exists now. The comments were originally added
in 7c0b742. I'm not sure what should be asserted here; from the history it seems like it refers to the tests above.

[rhshadrach]

Also note those comments still exist in the test immediately below.

[jbrockmendel]

if we want to coerce to object (and i think we do), then for starters the insert on L467 shouldn't raise but should instead be equivalent to obj.astype(object).insert(1, 1). so i think can just change that and then xfail it?

the harder question is what we do in the two cases above that with mismatched tz or mismatched tzawareness. with mismatched tz could just cast (#37605), with mismatched tzawareness could also cast to object.

[jreback]

if you can create an issue for this in followup (and PR if you can!)

[rhshadrach]

@jbrockmendel I adjusted the coercion test, let me know what you think.

For the offset test, the value 1000 causes the OutOfBounds exception for Week, Month, etc but not Day. Since the exception is caught, this makes the Day test pass even on 32-bit. One possible resolution is to move the xfail test down (current state of this PR), but I think better would be to find appropriate numbers for the different classes so that we can remove the except OutOfBounds entirely. Wanted to see what you thought first before I did so.

[rhshadrach]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jreback]

lgtm. cc @jbrockmendel

[rhshadrach]

I've looked into implementing my suggested approach in #38960 (comment) but don't see any way of deducing a good value except hard coding on a case-by-case basis. I'll do so if that's desired, but it doesn't seem like a good change to me.

[jbrockmendel]

way of deducing a good value

# for cls in pd.offsets.Tick.__subclasses__():
cls = pd.offsets.Day
bound = np.iinfo(np.int64).max  # might be np.intp if 32bit is the issue?
nmax = bound // cls(1).nanos

pd.Timedelta(cls(nmax))  # <- works
pd.Timedelta(cls(nmax + 1))   # <- raises

is that what you're looking for?

[jreback]

if you can create an issue for this in followup (and PR if you can!)

[rhshadrach]

Thanks @jbrockmendel, that is indeed what I was going for. However reviewing the test again, I think I'm confused on what this test is trying to do. From the comment:

try to create an out-of-bounds result timestamp; if we can't create
the offset skip

I was under the impression that the test would create an offset large enough so that when it was added to the TimeStamp it would raise an OutOfBoundsDatetime. If this was the case, then one would be expecting the line

result = Timestamp("20080101") + offset

to raise. But there are several lines of testing after this, which is why I'm pretty sure I'm missing the point of this test.

[rhshadrach]

I was thinking the test could be written something like this:

        # try to create an out-of-bounds result timestamp

        ts = Timestamp("20080101")
        max_seconds = (ts.max - ts).total_seconds()
        offset_100 = self._get_offset(self._offset, value=100)
        seconds = ((ts + offset_100) - ts).total_seconds() / 100
        # Add a small buffer since we're taking an average
        value = int(max_seconds / seconds) + 10
        offset = self._get_offset(self._offset, value=value)

        with pytest.raises(OutOfBoundsDatetime):
            _ = ts + offset

[jbrockmendel]

@rhshadrach sorry this fell through the cracks of my inbox. is the _get_offset thing still an issue?

[rhshadrach]

@jbrockmendel - Not a problem, thanks for getting back. I'm really not sure - if my understanding of the test is correct, I think it could be streamlined a bit (and I'd be happy to do), but I have strong suspicions I don't really understand what the test is testing. I tried to explain this in #38960 (comment). If that comment isn't clear, let me know and I can try again.

[jbrockmendel]

But there are several lines of testing after this, which is why I'm pretty sure I'm missing the point of this test.

Best guess, IIRC there was at one point (maybe still is) some part of the offsets code that would work with pydatetimes instead of Timestamps. Could be that part of the test was intended for those?

[rhshadrach]

Thanks @jbrockmendel - I finally was able to track down the original implementation of this test in #5327:

    def test_apply_out_of_range(self):
        if self._offset is None:
            raise nose.SkipTest("_offset not defined")

        # try to create an out-of-bounds result timestamp; if we can't create the offset
        # skip
        try:
            offset = self._offset(10000)

            result = Timestamp('20080101') + offset
            self.assert_(isinstance(result, datetime))
        except (OutOfBoundsDatetime):
            raise
        except (ValueError, KeyError):
            raise nose.SkipTest("cannot create out_of_range offset")

Notice here we raise on OutOfBoundsDatetime. The release note from that PR was:

Allows operating of Timestamps to return a datetime if the result is out-of-bounds related

The test I proposed above passes all checks (namely, can always construct an offset that when added to a Timestamp will raise an OutOfBoundsDatetime). I'm thinking that the behavior of returning a datetime is no longer the case and wondering if the test above should be used instead.

[jbrockmendel]

I'm thinking that the behavior of returning a datetime is no longer the case and wondering if the test above should be used instead.

there may be some legacy compat code in there, but i don't think we support pydatetime in any kind of consistent way at this point