-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: Check for ModuleNotFoundError on Python 3.6+ #2136
Conversation
Looks legit however, it would be nice to squeeze in the changes for #2134 here too? Travis supports the Could also take a look if you want 👍 |
while i normally would disagree to mix infrastructure changes and code change |
I am able to enable Travis, but I have zero experience with appveyor |
@hroncok same as travis, add |
@ignatenkobrain Figured that out. First try, let's see what the CIs will say. Also not sure whether you want py36-trial and so on. And if so, whether I need to copy paste it around in tox.ini (seems ugly). |
Simply adding py36 to .travis.yml will not work. Need to change the matrix to use 3.6-dev. |
If AppVeyor proves difficult, I think we can hold until 3.6 is officially released, testing only on Travis for now.
I agree, specially keeping them in separate commits (which is the case already).
Let's keep testing py35-trial and etc. for now. When 3.6 is officially released we will want to update the other test environments to use the latest version if possible. |
Yep, it these Python versions are installed on AppVeyor:
As I said, I'm in favor of just skipping testing 3.6 on AppVeyor until it is available as part of the installation. |
In that case, only enabling on Travis CI. Amended. |
Thanks a ton @hroncok! |
Cool 👍 |
@@ -227,7 +227,7 @@ def test_doctest_unex_importerror_with_module(self, testdir): | |||
# doctest is never executed because of error during hello.py collection | |||
result.stdout.fnmatch_lines([ | |||
"*ERROR collecting hello.py*", | |||
"*ImportError: No module named *asdals*", | |||
"*Error: No module named *asdals*", |
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.
I think it might be good to keep the match strict here.
a) It could be changed to cause an ImportError for py36, too.
b) it could be asserted looking at result.stdout.lines
directly, using a regular expression.
c) there could be an alternative check for py36 and later.
I would prefer c).
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.
I'm not sure @blueyed, it seems a fuzzy matching for the message here is enough. Do you see a situation where we obtain a match but it is the wrong match? Seems unlikely to me.
But I'm not entirely against the idea either, if @hroncok agrees this can easily be changed to:
expected_exc = 'ModuleNotFoundError' if sys.version_info[:2] >= (3, 6) else 'ImportError'
result.stdout.fnmatch_lines([
"*ERROR collecting hello.py*",
"*%s: No module named *asdals*" % expected_exc,
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.
Amended
Those tests originally checked for ImportError. Since Python 3.6 ModuleNotFoundError is raised in this context instead, the tests didn't work as they are text based (so exception inheritance does not save the day). Fixes pytest-dev#2132
Partial fix for pytest-dev#2134
(Failed CI tests seem unrelated to me.) |
@hroncok, pushed a commit that moved the compatibility code to |
This tests originally checked for ImportError. Since Python 3.6
ModuleNotFoundError is raised in this context instead, the test didn't work
as it is text based (so exception inheritance does not save the day).
The test now simply checks for any *Error, which makes it less specific,
but still valuable.
Fixes #2132
I consider this a trivial fix (I realize it's not in documentation, but it's in tests, so I think the same logic applies).