improve test_copy_reduce and test_deepcopy_reduce coverage

[dslavicek]

Line 94 in copy() method and line 150 in deepcopy() method were not covered by tests. This happened because the reductor = getattr(x, "__reduce_ex__", None) at lines 88 and 144 took __reduce_ex__ from the object class which preventing reaching the else block where __reduce__ is checked.

This PR adds coverage for those two lines and by this increases branch coverage of the copy module to 100%.

printscreen of coverage check on main:

[tomasr8]

Rather than modifying existing tests, could you create new tests for the __reduce__ case?

[dslavicek]

Rather than modifying existing tests, could you create new tests for the __reduce__ case?

Since the existing tests are named test_copy_reduce and test_deepcopy_reduce it occured to me they are supposed to cover the __reduce__ case and there should be no new tests added for this.

Maybe I gave the PR a confusing name and should have named it something like "Fix test_copy_reduce and "test_deepcopy_reduce".

[tomasr8]

Ah I see, I didn't notice there was already a test for __reduce_ex__.
Could you also add this assert to mirror what was done in the __reduce_ex__ tests?

def __reduce_ex__(self):
    self.fail("shouldn't call this")

[dslavicek]

Ah I see, I didn't notice there was already a test for __reduce_ex__. Could you also add this assert to mirror what was done in the __reduce_ex__ tests?

def __reduce_ex__(self):
    self.fail("shouldn't call this")

I don't think I can. The point is copy and deepcopy attempt to get attribute __reduce_ex__ first (copy on line 88, deepcopy 144 of copy.py) and only if they do not find __reduce_ex__ they try to get __reduce__ (lines 92 and 148 respectively). The aim of the modified tests is to test that __reduce__ is called after an unsuccessful attempt to get attribute __reduce_ex__.

In the existing version of the test because no __reduce_ex__ is defined for the class C, getattr takes __reduce_ex_ from base object class (see Objects/typeobject.c lines 8218-8249). The object.__reduce_ex__ method calls the C classes __reduce__ from within and reudctor = getattr(x, "__reduce__", None) from the copy function (line 92) does not get called.

I want to make sure getattr(x, "__reduce_ex__", None) returns None, so that the if block on line 89 is skipped and the else block on line 91 is entered.

[tomasr8]

I don't think I can. The point is copy and deepcopy attempt to get attribute reduce_ex first

This should work no? Unless I'm missing something?

class C(object):
    def __reduce_ex__(self, proto):
        self.fail("shouldn't call this")
    def __reduce__(self):
        c.append(1)
        return ""
    def __getattribute__(self, name):
        if name == "__reduce_ex__":
            raise AttributeError(name)
        return object.__getattribute__(self, name)

Also, unrelated, but the self.fail call in test_copy_reduce_ex seems wrong to me. The __reduce__ method is shadowing the outer self so self.fail will not call the right thing here. I think the __reduce__ method parameter should be renamed.

[dslavicek]

I don't think I can. The point is copy and deepcopy attempt to get attribute reduce_ex first

This should work no? Unless I'm missing something?

class C(object):
    def __reduce_ex__(self, proto):
        self.fail("shouldn't call this")
    def __reduce__(self):
        c.append(1)
        return ""
    def __getattribute__(self, name):
        if name == "__reduce_ex__":
            raise AttributeError(name)
        return object.__getattribute__(self, name)

Also, unrelated, but the self.fail call in test_copy_reduce_ex seems wrong to me. The __reduce__ method is shadowing the outer self so self.fail will not call the right thing here. I think the __reduce__ method parameter should be renamed.

Sorry, I misunderstood. Of course I can add the __reduce_ex__ when I keep the __getattribute__ override. I added the methods you requested.

[tomasr8]

Ah sorry, I wasn't clear about what I meant.. I'd just fix the self.fail shadowing, but otherwise it looks good to me :)

[dslavicek]

Ah sorry, I wasn't clear about what I meant.. I'd just fix the self.fail shadowing, but otherwise it looks good to me :)

I am unsure how to handle it. Should I just rename the parameter? What new name do you propose? Should I replace self.fail("...") with assert False, "..."?

[tomasr8]

I think renaming the inner self parameter should be fine. Maybe you could even make it just *args?

def __reduce_ex__(*args):
    self.fail("shouldn't call this")

[dslavicek]

I think renaming the inner self parameter should be fine. Maybe you could even make it just *args?

def __reduce_ex__(*args):
    self.fail("shouldn't call this")

Done :-)

[tomasr8]

Also needs to be fixed in test_copy_reduce and test_deepcopy_reduce ;)