Hook for preprocessing test module AST prior to rewriting

[Sup3rGeo]

Hi,

We are building a nice reporting framework around pytest and we would like to extend the approach taken when rewriting assertions to whatever other things we find interesting - basically providing more information without needing to change the source code.

For this I would propose a new hook called pytest_ast_preprocess(tree, fn, config). It basically has the same signature as assertion.rewrite.rewrite_asserts function, and is meant to delegate this AST processing to a plugin.

It is called preprocess because it is supposed to act before the rewrite_asserts function itself:

try:
    tree = ast.parse(source)
except SyntaxError:
    # Let this pop up again in the real import.
    state.trace("failed to parse: %r" % (fn,))
    return None, None
hook.pytest_ast_preprocess(tree,fn,config)   # not sure proper way to call hook
rewrite_asserts(tree, fn, config)
try:
    co = compile(tree, fn.strpath, "exec", dont_inherit=True)
except SyntaxError:
    # It's possible that this error is from some bug in the
    # assertion rewriting, but I don't know of a fast way to tell.
    state.trace("failed to compile: %r" % (fn,))
    return None, None

Thanks!

[pytestbot]

GitMate.io thinks possibly related issues are #2203 (Exclude some test modules from assertion rewriting), #2419 (Rewrite hook attempts to rewrite namespace package), #1978 (pytest not finding any tests under module), #427 (Option to have pytest rewrite assertions in additional non-test modules), and #46 (adding hooks for controlling instances of tests).

[nicoddemus]

I'm 👍: it is a trivial change to the core and opens possibilities for plugins to do fancy stuff with it.

[RonnyPfannschmidt]

this is strictly a massive problem, since this creates a setup where ast caching no longer reliably matches the desired ast

im -1 on it unless we present a way to make it consistent per project + prevent inter-project ast rewriting issues (as they can happen for shared editable installs)

[nicoddemus]

where ast caching no longer reliably matches the desired ast

What do you mean @RonnyPfannschmidt ? We don't cache the AST, we only cache the generated code (well after AST has been processed).

[RonnyPfannschmidt]

whops - i intended to talk about the cached code objects

[nicoddemus]

I see, thanks. So the proposal is fine then, right? After all is just an extra hook which can alter the AST just like pytest itself does.

[RonnyPfannschmidt]

@nicoddemus the proposal as is is unacceptable as it creates the issues i mention without a solution, just having that hook simply creates a massive liability and there is no chance for that api to ever be usable correctly

[nicoddemus]

Sorry I fail to see the problem 😕

Currently pytest does roughly this:

1. Parses the file into an AST
2. Rewrites the AST, manipulating the tree
3. Compiles the AST to bytecode
4. Caches and returns the module

This proposal changes the flow to this IIUC:

1. Parses the file into an AST
2. Rewrites the AST, manipulating the tree
3. Calls a hook passing the tree, which gives plugins the opportunity to manipulate the tree as well
4. Compiles the AST to bytecode
5. Caches and returns the module

Can you elaborate what are the problems with the above?

[Sup3rGeo]

I guess what @RonnyPfannschmidt could be talking about is that, once the modules are compiled for the first time, the AST rewriting part is actually never called again.

I can see this becoming a problem (actually hook users will see this necessarily) if the hook implementation changes in a plugin and user forget to clean __pycache__ folder. But I think this could just be well documented.

Now if I understood well enough, are you talking about running the same test in a session with one AST rewriting plugin, and then in another session another AST rewriting plugin tries to run the same tests and ends up actually not rewriting because of the cached files?

[RonnyPfannschmidt]

@Sup3rGeo correct- if the ast rewriters that acted as part of the system are not considered on cached module loading, this will under guarantee create issues that are next to impossible to debug

[Sup3rGeo]

Can we detect from pluginmanager that there exists a plugin/conftest that implemented a specific hook?

If possible, one idea would be to detect that this hook if implemented by at least one plugin and, if so, just don't read nor save any pyc files at all, always rewriting the modules.

• If the hook is not implemented in any plugin, this leaves everything untouched
• If the hook is implemented, then we can just document the possible performance penalty of not using any cache files. This would be something easy to live with in most cases I guess.

[Sup3rGeo]

might be related to #1680

[asottile]

If you're interested in a black-magic workaround, here's a pytest test which rewrites itself before executing: https://github.com/asottile/future-fstrings/blob/f06c03e5817aeb9fdccc1b4b6728dec600ce137b/tests/future_fstrings_test.py#L1

EDIT: though even that falls victim to the pyc problem mentioned above: https://github.com/asottile/future-fstrings/blob/f06c03e5817aeb9fdccc1b4b6728dec600ce137b/tox.ini#L9-L10

[Zac-HD]

Closing this issue because, while it would be a nice feature, it would also be very difficult to use safely or debug when it goes wrong 😕

[RonnyPfannschmidt]

just a extra note - instead of a hook, its thinkable that we can consider a explicitly listed set of ast rewrite addons, which in turn take part in deciding the filename