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

tests: Consolidate version (2 vs. 3) and platform (CPython vs. PyPy) checks #2376

Merged

Conversation

EricCousineau-TRI
Copy link
Collaborator

Minor updates that may help discussion in #2366

@EricCousineau-TRI
Copy link
Collaborator Author

FWIW I dunno how much I love injecting stuff into pytest, but 🤷, it at least sticks with convention.
Perhaps at some point it would make things more obvious to consolidate these into an explicit utility module, like pybind11_test_utilities or something.

@EricCousineau-TRI EricCousineau-TRI force-pushed the feature-py2-py3-test-checks branch from 2c81360 to 134f3e7 Compare August 9, 2020 17:21
tests/conftest.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

A few minor remarks, but I like this! :-)
Question remains what to do with @rwgk's tests, since he prefers bytes is str when working with the string/bytes types: #2366 (comment).

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Thanks Eric for taking the initiative!

tests/conftest.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@YannickJadoul
Copy link
Collaborator

Any further suggestions how to continue with this? It would be nice to get this in so new PRs can use this.

@henryiii
Copy link
Collaborator

Since we already inject stuff into pytest with pytest_configure, I'd be okay to put this in for now, then do a proper cleanup later. I bet that's what @EricCousineau-TRI meant by convention. There are several bad practices here that need to be fixed, but that's not the focus of this PR. The only change I think we should require now is avoiding PY3 - I can add that, though.

@henryiii
Copy link
Collaborator

I've pushed the PY2 only change, and dropped the fail fasts. @EricCousineau-TRI, does this look fine? I'll try to refactor later for pytest best practices, but it shouldn't hold up this PR.

@henryiii henryiii force-pushed the feature-py2-py3-test-checks branch from 3f463f1 to a12420e Compare August 14, 2020 16:10
@henryiii
Copy link
Collaborator

Sorry, had to rebase & force-push, another check that needs to be converted to PY2 was added, and it was detected by flake8.

@YannickJadoul
Copy link
Collaborator

Great, thanks, @henryiii! As far as I'm concerned, this can go in soon enough, then?

@EricCousineau-TRI
Copy link
Collaborator Author

@EricCousineau-TRI, does this look fine?

Yup! Just going through to address the open discussions.

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Aug 14, 2020

K, pending CI, this should be good to go! Thank y'all!

@EricCousineau-TRI
Copy link
Collaborator Author

Mac CI going slow :( Looks like there're only 2 nodes available for provisioning... 😿

@EricCousineau-TRI EricCousineau-TRI merged commit ebdd0d3 into pybind:master Aug 14, 2020
@YannickJadoul
Copy link
Collaborator

Hurray! :-)

@henryiii henryiii added the ci related to the CI system label Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci related to the CI system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants