-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Collect any tests from a package's __init__.py #3797
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
Conversation
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 @jonozzz, could you please add a test and a CHANGELOG entry?
Restarting CI manually due to Travis hiccup |
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.
Missing a test here, gentle ping 😁
@nicoddemus I'm actually waiting on @deeechun to answer. I don't think this was possible in pre-3.7.0, wasn't it? |
I can get tests in
Running this in
And on
The key thing to remember is to set |
Oh I just saw that @deeechun replied with basically the same thing. 😁 |
@@ -596,6 +596,7 @@ def isinitpath(self, path): | |||
def collect(self): | |||
this_path = self.fspath.dirpath() | |||
pkg_prefix = None | |||
yield Module(this_path.join("__init__.py"), self) |
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.
as is this isnt acceptable, since it doesn't check if the filenames is acceptable and might cause surprises for people that have objects with fitting names in packages
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.
Ahh good catch @RonnyPfannschmidt. Just to be clear you mean to change this bit to something like:
init_module = this_path.join("__init__.py")
if init_module.check(file=1) and init_module.match(self.config.getini('python_files')):
yield Module(init_module, self)
?
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.
we should reuse the filename matching and we dont need to check for existence anymore
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.
Right, thanks!
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.
Well, the __init__.py
is hardcoded at:
Line 204 in d0bd01b
for pat in parent.config.getini("python_files") + ["__init__.py"]: |
If you remove that, then you have to explicitly add the
__init__.py
pattern to "python_files" to enable the Package collection. I'm not sure what's the best way to look for packages but also not duplicate the filename matching.
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.
Gentle ping here? I'm not sure if it is worth reusing the filename matching for __init__
files, which is what I got from @RonnyPfannschmidt's comment.
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.
packages have different behaviours than modules to begin with, so i dont see a problem there
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.
So we can go ahead and use my suggestion?
init_module = this_path.join("__init__.py")
if init_module.check(file=1) and init_module.match(self.config.getini('python_files')):
yield Module(init_module, self)
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.
yes
This was marked as merged but the actual code did not go in; probably some commits from this went into #3861, but the final code does not contain the changes here. |
Fixes #3773