BUG: test_finalize.py tests failing on master with pytest-randomly

[jbrockmendel]

On Ubuntu18 im seeing a variable number of tests in test_binops failing with empty .attrs

cc @TomAugspurger

[TomAugspurger]

This is on CI or just locally? Any tracebacks you can share?

[jbrockmendel]

Just locally

____________________________________ test_binops[rmul-args0-right] _____________________________________

args = (1, 0    1
dtype: int64), annotate = 'right'
all_arithmetic_functions = <function rmul at 0x7f4a5c3df400>

    @pytest.mark.parametrize("annotate", ["left", "right", "both"])
    @pytest.mark.parametrize(
        "args",
        [
            (1, pd.Series([1])),
            (1, pd.DataFrame({"A": [1]})),
            (pd.Series([1]), 1),
            (pd.DataFrame({"A": [1]}), 1),
            (pd.Series([1]), pd.Series([1])),
            (pd.DataFrame({"A": [1]}), pd.DataFrame({"A": [1]})),
            (pd.Series([1]), pd.DataFrame({"A": [1]})),
            (pd.DataFrame({"A": [1]}), pd.Series([1])),
        ],
    )
    def test_binops(args, annotate, all_arithmetic_functions):
        # This generates 326 tests... Is that needed?
        left, right = args
        if annotate == "both" and isinstance(left, int) or isinstance(right, int):
            return
    
        if isinstance(left, pd.DataFrame) or isinstance(right, pd.DataFrame):
            pytest.xfail(reason="not implemented")
    
        if annotate in {"left", "both"} and not isinstance(left, int):
            left.attrs = {"a": 1}
        if annotate in {"left", "both"} and not isinstance(right, int):
            right.attrs = {"a": 1}
    
        result = all_arithmetic_functions(left, right)
>       assert result.attrs == {"a": 1}
E       AssertionError: assert {} == {'a': 1}
E         Right contains 1 more item:
E         {'a': 1}
E         Use -v to get the full diff

[jbrockmendel]

It looks like i have (had) pytest-randomly installed on this machine. Removing it fixes these failures. So it looks like there's some global state somewhere

[jbrockmendel]

@TomAugspurger i thought i pinged on this last week but can't find it. I think there may be a simple solution here, need you to confirm if it is correct.

Am I right in thinking the following should always hold:

def test_binops(args, annotate, all_arithmetic_functions):
    # This generates 326 tests... Is that needed?
    left, right = args
    if annotate == "both" and isinstance(left, int) or isinstance(right, int):
        return

+    if isinstance(left, (pd.Series, pd.DataFrame)):
+        assert left.attrs == {}
+    if isinstance(right, (pd.Series, pd.DataFrame)):
+        assert right.attrs == {}

[TomAugspurger]

It seems a bit strange to make an assertion about the inputs to the test, no? But I guess the issue is that they're being unexpectedly mutated somehow?

[jbrockmendel]

But I guess the issue is that they're being unexpectedly mutated somehow?

exactly. the same Series/DataFrame objects are being re-used across the parametrized tests, so when the attrs are set in the lines below, this assumption is invalidated in the following tests.

so if its correct that these assertions should hold, then we can just clear the attrs at the beginning of each test and be fine

[jbrockmendel]

related, i think a possible typo?

        if annotate in {"left", "both"} and not isinstance(left, int):
            left.attrs = {"a": 1}
-        if annotate in {"left", "both"} and not isinstance(right, int):
+        if annotate in {"right", "both"} and not isinstance(right, int):
            right.attrs = {"a": 1}
``

[jbrockmendel]

@TomAugspurger gentle ping. if im right about the expected behavior, ive got a branch ready to push

[TomAugspurger]

Ah I see, you're using the asserts to show the expected behavior. The diff would be to set the attrs (and maybe make a copy too?).

If so, yes I agree, and that does seem like a typo on the second if annotate in ....

[jbrockmendel]

Well I take back the part about the branch being ready... a few weeks ago I thought I had this working, but now clearing out the attrs at the start of the test breaks 42 tests (regardless of whether I change the "left" -> "right")