cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-94808: add tests covering `PySequence_[InPlace]Concat`

Open sobolevn opened this issue 3 years ago • 1 comments

Key points:

  1. I've covered these two C-API function calls by covering operator module, 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.
  2. I've followed the module style: it has big tests cases for operators, this is why I didn't bother creating a new spec or separate test cases for each sub-case
  • Issue: gh-94808

sobolevn avatar Nov 10 '22 09:11 sobolevn

There's an unrelated test failure:

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\a\1\b\test\test_python_3768�\test_python_worker_5520�'

sobolevn avatar Nov 10 '22 12:11 sobolevn

cc @gvanrossum

kumaraditya303 avatar Nov 24 '22 12:11 kumaraditya303

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-bot avatar Nov 29 '22 18:11 bedevere-bot

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. I agree that most of them do not make much sense.

So, I've made several changes to this PR:

  1. I've added more cases for subtypes to cover all combinations: subtype + subtype, subtype + parent, parent + subtype. It does not really affect coverage, but it feels like a right thing to do
  2. I've remove all type assertions except: results, arguments that do mutate (iconcat and list)
  3. I've changed type assertions for tuple and list to be exact, for example: self.assertIs(type(res), tuple)

So, I hope it should be good now! Thanks again for the review, it helped a lot!

sobolevn avatar Mar 24 '23 12:03 sobolevn

I have made the requested changes; please review again

sobolevn avatar Mar 24 '23 12:03 sobolevn

Thanks for making the requested changes!

@kumaraditya303, @gvanrossum: please review the changes made to this pull request.

bedevere-bot avatar Mar 24 '23 12:03 bedevere-bot

Please find another reviewer, I'm on vacation.

gvanrossum avatar Mar 24 '23 15:03 gvanrossum

Happy vacation 🏖️ 😊

sobolevn avatar Mar 24 '23 16:03 sobolevn