gh-94808: add tests covering PySequence_[InPlace]Concat
#99319
Conversation
|
There's an unrelated test failure:
|
|
cc @gvanrossum |
Lib/test/test_operator.py
Outdated
| self.assertEqual(operator.concat(data1, data2), | ||
| ListSubclass([1, 2, 'a', 'b'])) | ||
| self.assertEqual(operator.concat(data1, data2), data1 + data2) | ||
| self.assertEqual(data1, ListSubclass([1, 2])) # must not change | ||
| self.assertEqual(data2, ListSubclass(['a', 'b'])) # must not change |
There was a problem hiding this comment.
This doesn't prove much. The results are in all cases list instances, not ListSubclass instances, because of the way list operator overloading works -- data1+data2 is a plain list, and __eq__ considers a plain list equal to a list subclass with the same elements. The test makes it appear differently.
There was a problem hiding this comment.
Good catch, thanks! I've totally missed that. I've added several assertIsInstance checks and updated the expected values.
Lib/test/test_operator.py
Outdated
| self.assertEqual(operator.concat(data1, data2), data1 + data2) | ||
|
|
||
| res = operator.concat(data1, data2) | ||
| self.assertIsInstance(res, list) |
There was a problem hiding this comment.
This doesn't prove anything, does it? Whether the result is a ListSubclass or a plain list, this will always be true. If you want to say something interesting here I'd assert that it isn't a ListSubclass instance.
Lib/test/test_operator.py
Outdated
| res = operator.concat(data1, data2) | ||
| self.assertIsInstance(res, list) | ||
| self.assertEqual(res, [1, 2, 'a', 'b']) | ||
| self.assertIsInstance(data1, ListSubclass) |
There was a problem hiding this comment.
I don't see the point of this -- of course data1 and data2 are instances of ListSubclass, that's how they were created.
| TupleSubclass(['a', 'b', 1, 2])) | ||
| self.assertEqual(operator.concat(data1, data2), data1 + data2) | ||
| res = operator.concat(data1, data2) | ||
| self.assertIsInstance(res, TupleSubclass) |
Lib/test/test_operator.py
Outdated
| res = operator.concat(data1, data2) | ||
| self.assertIsInstance(res, TupleSubclass) | ||
| self.assertEqual(res, TupleSubclass(['a', 'b', 1, 2])) | ||
| self.assertIsInstance(data1, TupleSubclass) |
There was a problem hiding this comment.
But this is still questionable. I can't think of a scenario where this would fail.
Or do you have coverage results showing this is needed? (Where?)
Ditto for data2 and again in the following block of tests.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Sorry, it took me quite longer than I expected to get back to it :) I've addressed your main review points. I went with very defensive asserts in this case. So, I've made several changes to this PR:
So, I hope it should be good now! |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @kumaraditya303, @gvanrossum: please review the changes made to this pull request. |
|
Please find another reviewer, I'm on vacation. |
|
Happy vacation 🏖️ 😊 |
Key points:
operatormodule, which uses these functions in their C implementation. I think that this is better because it also tests a lot of other things, not just a single API function.