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

Assert rewrite causes reload() to fail #743

Closed
pytestbot opened this issue May 14, 2015 · 9 comments
Closed

Assert rewrite causes reload() to fail #743

pytestbot opened this issue May 14, 2015 · 9 comments
Labels
type: bug problem that needs to be addressed

Comments

@pytestbot
Copy link
Contributor

Originally reported by: BitBucket: lcampagn, GitHub: lcampagn


I have a unit test that passes when run with assert=plain or reinterp, but fails with rewrite. I also looked over #435, but I believe this is a separate issue. The offending test is attached. It works by writing a module to be imported, then re-writing, reloading it, and testing that the reload worked as expected.

I imagine there might be no sane way to fix this bug. If that is the case, then is there a way to disable assertion rewriting on a per-module basis? I was a bit surprised to find that adding messages to the assert statements did not fix the problem.


@pytestbot
Copy link
Contributor Author

Original comment by Floris Bruynooghe (BitBucket: flub, GitHub: flub):


There's an undocumented way of disabling the ast-rewriting for a module: write PYTEST_DONT_REWRITE in the module docstring.

@pytestbot
Copy link
Contributor Author

Original comment by BitBucket: lcampagn, GitHub: lcampagn:


Adding the docstring to the top of the module does not chance the output of the attached script for me. Is there something else I need to enable pytest to check the docstring?

@pytestbot
Copy link
Contributor Author

Original comment by Floris Bruynooghe (BitBucket: flub, GitHub: flub):


Right, I looked at the code and while it disables rewriting of the assertions it still generates the py.test specific bytecode file. So you'd still not have reload work. Not yet sure what would be required to support reload properly.

Btw, looking at your script you probably don't really have to have this problem. You do realise you can replace a lot of the code in the example with the tmpdir fixture (http://pytest.org/latest/tmpdir.html). It would get rid of the temporary directory creation and atexit stuff quite easily. You might even be able to work around the reload things using the py.path.local.pyimport() method. But them I'm not sure how much this is an example demonstrating the reload issue vs what you want to achieve.

@pytestbot
Copy link
Contributor Author

Original comment by BitBucket: lcampagn, GitHub: lcampagn:


Thanks, the tpmdir fixture is a nice feature. However the reload() call is not optional for me; the code I posted above is just a minimal demonstration of the issue. For now I will stick to using assert=reinterp.

I wonder if it would be possible for the module rewriter to check for calls to reload() in addition to assertions, and insert a function call that would check for bytecode files that need to be rebuilt.

@pytestbot pytestbot added the type: bug problem that needs to be addressed label Jun 15, 2015
@RonnyPfannschmidt
Copy link
Member

do we have any more feedback on this, do we even want to try and support it?

@nicoddemus ?

@nicoddemus
Copy link
Member

Nice, thanks for the ping, didn't know about PYTEST_DONT_REWRITE.

For cross-reference: #2203.

I think it should be simple to make PYTEST_DONT_REWRITE also skip our AssertRewriteHook entirely, which perhaps would be enough for this example, although I can't say for sure until spending some more type on it.

Hmm it might hard to get more feedback on this from the OP, as there's no lcampagn user on GH and the BB user doesn't have any additional information that I can see at least.

Given that I think we can close this issue and continue the discussion over #2203 then.

@campagnola
Copy link

OP here; I am still looking for a solution. Happy to provide feedback or test ideas!

@RonnyPfannschmidt
Copy link
Member

@campagnola one thing i dont understand is why reload was needed, the other bit is i wonder how reload interacts with the module loading peps

@campagnola
Copy link

I just found a workaround which, in retrospect, is obvious: assertion rewriting is only done on files that are named like test modules, so all I had to do was pick a different name for the temporary module and it will be ignored by the rewriter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug problem that needs to be addressed
Projects
None yet
Development

No branches or pull requests

4 participants