[Python-checkins] cpython (merge 3.2 -> default): MERGE: Better test for Issue #15402: Add a __sizeof__ method to struct.Struct

[Meador Inge]

On Mon, Jul 23, 2012 at 11:38 AM, Jesus Cea <jcea at jcea.es> wrote:

>> As for the tests, I intentionally kept them the way that Serhiy
>> contributed them -- using >= instead of >.  I kept them this way
>> because we also discussed in issue14596 the prospect of optimizing
>> the way repeat counts are handled.  These tests would start failing
>> if (when) that optimization happens.
>
> The problem is that if we do ">=", then an unpatched python
> interpreter could pass the test too. So we are not actually testing
> the feature.

We are testing the feature because the first test looks like:

        self.assertGreater(sys.getsizeof(struct.Struct('BHILfdspP')),
                           sys.getsizeof(struct.Struct('B')))

The way things were written 'sys.getsizeof' would returns the same
answer regardless of the format string.

The remaining tests looked like:

        self.assertGreaterEqual(sys.getsizeof(struct.Struct('123B')),
                                sys.getsizeof(struct.Struct('B')))
        self.assertGreaterEqual(sys.getsizeof(struct.Struct('B' * 123)),
                                sys.getsizeof(struct.Struct('123B')))
        self.assertGreaterEqual(sys.getsizeof(struct.Struct('123xB')),
                                sys.getsizeof(struct.Struct('B')))

and while they didn't fail without the patch I felt they were still useful in
documenting that there is nothing that guarantees 'sizeof("123B") > sizeof("B")'
'sizeof("B" * 123) > sizeof("123B")', or 'sizeof("123xB") > sizeof("B")'.

> If the repeat counters are going to be optimized, the obvious step
> would be to upgrade the test to do something like "BHHIL" instead of
> "123B". I would wait until this feature is implemented to update the test.

That is what the first test basically already does :-)

> What do you think?.

It isn't that big of a deal.  We can just leave the tests as you changed them.
In the future it would probably be better to hash this stuff out in the tracker.
The patch was out for review for several days ...

Thanks,

-- Meador