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

Update config._getconftest_pathlist for pytest-dev #157

Merged
merged 6 commits into from
Jun 16, 2021

Conversation

saimn
Copy link
Contributor

@saimn saimn commented Jun 13, 2021

@saimn saimn requested a review from pllim June 13, 2021 17:32
@pllim
Copy link
Contributor

pllim commented Jun 15, 2021

Something else need changing to be compatible with pytest-dev

.../pytest_doctestplus/plugin.py:213: in collect
    module = fspath.pyimport()
E   AttributeError: 'PosixPath' object has no attribute 'pyimport'

@@ -203,10 +210,10 @@ def collect(self):
self.fspath)
else:
try:
module = self.fspath.pyimport()
module = fspath.pyimport()
Copy link
Contributor

Choose a reason for hiding this comment

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

This change broke something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that method doesn't exist on Path objects, so I will need to reimplement it I guess.

@saimn
Copy link
Contributor Author

saimn commented Jun 16, 2021

@pllim - Tests pass now ! Also I had forgotten about #156... see the discussion there about pyimport, I chose the "import the internal pytest function" solution, seems fine for now.

Comment on lines +470 to +471
except TypeError:
# Pytest 7.0+
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not doing version check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about it ;). And this is a bit of a mess, some changes are in 6.3 which is not released yet, and some will be in 7.0. And dev is currently 6.3.devXXX though it includes the 7.0 changes apparently.
So I added PYTEST_GT_6_3 after but I'm still not sure if that's a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean try/except is easy to do, distinguishing 6.3 and 7.0 is currently not obvious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, maybe we can merge as is but open issue to revisit in the future because you don't want the stable pytest to keep hitting the except block.

@pllim
Copy link
Contributor

pllim commented Jun 16, 2021

Also I had forgotten about #156

Hah, me too! 😸

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks!

@pllim pllim merged commit afc77eb into scientific-python:main Jun 16, 2021
@saimn saimn deleted the fix-pytest-dev branch June 16, 2021 22:02
@bsipocz bsipocz added this to the 0.10.0 milestone Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants