-
Notifications
You must be signed in to change notification settings - Fork 676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Additional tests for #3168 #3175
Additional tests for #3168 #3175
Conversation
…g acquired inside of implementations.
8d61c64
to
edfa048
Compare
edfa048
to
6fb22db
Compare
I am confused now: They do seem to pass in the CI? Do they fail only intermittently? Could you also check whether #3168 does resolve the issues you found? |
Sorry, your PR definitely passes all of those tests (as confirmed by the CI). I meant to say that those tests are crafted so that PyO3 don't pass them without your PR. |
Ah sorry, I missed the target branch of this PR! It is targetting #3168 and not Also sorry for not starting with that: Thank you for going the extra mile and packaging up your tests so we can include them upstream! |
First time contributor has agreed to the new licensing scheme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for this comprehensive set of tests.
I'll hold off merging this into #3168 until we have consensus that that approach is what we will pursue to avoid that this PR needs to be recreated if we decide to pursue another approach.
89a6c43
to
08bdc32
Compare
These were a part of tests I was preparing for #3165, and I believe it's worthy to add them (any single of them fails in the current main branch).