Rewrite asserts in test-modules loaded very early in the startup #1787
Conversation
|
cc @flub |
| # modules not passed explicitly on the command line are only | ||
| # rewritten if they match the naming convention for test files | ||
| for pat in self.fnpats: | ||
| if fnmatch(fn_pypath.basename, pat): |
There was a problem hiding this comment.
Why the switch away from fn_pypath.fnmatch(pat)? Does this have to do something with the session thing?
There was a problem hiding this comment.
fn_pypath.fnmatch(pat) would trigger an fnmatch import, which would in turn cause this method to be called recursively until it blew up the stack.
There was a problem hiding this comment.
Ah, so that's the recursive bit then! I'd be in favour of putting that exact sentence in the comment just before, I think it's non obvious and warrants explaining.
|
Looks good to me, nice debugging! |
| return True | ||
|
|
||
| for marked in self._must_rewrite: | ||
| if marked.startswith(name): |
There was a problem hiding this comment.
Hmm just noticed this, but shouldn't this be if name.startswith(marked):? I'm supposing self._must_rewrite is a list of module prefixes like "pytest_qt", intended to match pytest_qt and any submodule.
There was a problem hiding this comment.
The _must_rewrite set can also be full module paths, like myplugin.plugin. This way round myplugin.plugin.foo or myplugin.plugin.__init__ would not be rewritten but if it was the other way round it would be. I guess it could just be a simple == comparison though?
There was a problem hiding this comment.
However thinking about it some more maybe name.startswith(marked) would be better behaviour. We should probably also update the docstring in assertion/__init__.py;register_assert_rewrite to read "will make sure that this module or all modules inside the package will get their assert statements rewritten" in that case though.
The only downside of that way round that I can think of is that someone may want to register mypkg.__init__ explicitly rather then just mypkg, but that's probably ok.
Now match modules which start with any of the names registered using register_assert_rewrite as discussed in pytest-dev#1787
nicoddemus
force-pushed
the
fix-rewrite-conftest
branch
from
c0dd385
to
41c1f63
Compare
Also now match modules which start with any of the names registered using register_assert_rewrite as discussed in pytest-dev#1787 Fix pytest-dev#1784
|
Implemented all of @flub's suggestions (thanks!) and squashed the commits. I will merge this later unless anybody else wants more time to review this further. 😁 |
Also now match modules which start with any of the names registered using register_assert_rewrite as discussed in pytest-dev#1787 Fix pytest-dev#1784
nicoddemus
force-pushed
the
fix-rewrite-conftest
branch
from
41c1f63
to
6711b1d
Compare
I investigated this some more and found the problem:
The assertion-rewrite hook is installed very early on, during the construction of the
Configobject. After installing the hook and still duringConfig's construction, plugins andconftestfiles are also imported and are rewritten ifAssertionRewriteHook._should_rewritereturnsTrue.Now the problem with the example posted in #1784 is that it imports
test_foocompareat the top-level of theconftest.pyfile which is loaded very early, when thesessionobject has not even been created yet (much less set on the hook), so it doesn't try to rewrite that module.Looking at the code I noticed that the code block that should handle this, the
for pat in self.fnpathsloop, doesn't really depend onself.sessionat all except for that weird comment about a cycle. Changing the code to always check if the filename matches one of the test patterns seems to do the trick.Fix #1784