Skip to content
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

Using recently added pytest.PY2 instead of str is bytes. #2396

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Aug 14, 2020

Important gain: uniformity & therefore easier cleanup when we drop PY2 support.
Very slight loss: it was nice to have str is bytes as a reminder in this specific context.

Important gain: uniformity & therefore easier cleanup when we drop PY2 support.
Very slight loss: it was nice to have `str is bytes` as a reminder in this specific context.
@rwgk rwgk requested a review from EricCousineau-TRI August 14, 2020 20:22
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@EricCousineau-TRI
Copy link
Collaborator

Failure here is for Python 3.9-dev on macOS:
https://github.com/pybind/pybind11/pull/2396/checks?check_run_id=986425511#step:14:56

     def test_python_to_cpp_to_python_from_thread_multiple_parallel():
        """Makes sure there is no GIL deadlock when running in a thread multiple times in parallel.
    
        It runs in a separate process to be able to stop and assert if it deadlocks.
        """
>       assert _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True) == 0
E       assert -11 == 0
E        +  where -11 = _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True)

test_gil_scoped.py:65: AssertionError

I say this is good to merge, as the failure is unrelated to this PR?

@EricCousineau-TRI
Copy link
Collaborator

I'ma go ahead and pull the "merge" trigger.

@rwgk
Copy link
Collaborator Author

rwgk commented Aug 14, 2020

Awesome, thanks Eric!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants