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

Proof of concept of fixtures as describe block funcargs #39

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ROpdebee
Copy link
Contributor

@ROpdebee ROpdebee commented Aug 18, 2021

I mentioned in #38 that I'd send a WIP PR if I managed to find some time, but of course I forgot. I had some time to do some hacking this evening, and I eventually arrived at a version that sort of achieves what I had hoped, so here it is. It's far from complete, there are numerous test failures and TODOs, in fact, but it's a start.

To summarise, what I did is capture the describe block's arguments using inspect and calling it with dummy arguments which store the argument name to gather the local definitions. When those dummy arguments are used in a test spec, they're added to the spec's closure, but we need the name as that would be the only way to match fixture values to the closure cells. Then I wrap each spec inside of a new function which loads the fixture values and either passes them as a normal argument, or temporarily modifies the closure to inject it. I've also added a sort of defence mechanism to prevent dereferencing the dummy arguments in the describe block itself, as that would lead to unexpected behaviour. This is done by scanning the function's bytecode for certain opcodes that load those arguments, but it's not thoroughly tested yet and likely not 100% reliable.

The main TODOs:

  • If fixtures can be injected into specs, should they also be injected into other fixtures nested in the describe block (if necessary)? Currently, they're just skipped.
  • More importantly, this completely breaks with @pytest.mark.parametrize, as that scans the test spec's params to check if it actually uses the parametrized variable. However, we're returning the wrapper, which doesn't include those params, so that fails. This could be fixed with some more meta-hacking or hooking more into pytest itself. hypothesis faced a similar problem and solved it by dynamically defining a new function with the adapted signature using exec.
  • Significantly more testing is required.
  • How this interacts with shared behaviours remains to be seen. I don't think it would make much sense to allow arguments on shared behaviour blocks, as that would require somehow injecting those arguments into the local scope of the @behaves_like describe block. I don't want to rule out that possibility, but that could easily get very messy very quickly.

Smaller TODOs:

  • This might break with kw-only arguments or arguments with default values. I'm not sure whether they'd make sense in this context, though.
  • It only supports the default describe prefix for now, as I didn't bother passing around config yet.
  • There's a function that checks whether the closure hacking is actually possible in the given Python version, but I'm not using it yet. It should be used to raise an error at collection time already.
  • Perhaps an easier way to do this would be to rewrite the AST, but that's still very complex and I'm concerned about the effect on coverage tools.

All in all, I'm of the opinion that this is all some very cool and interesting meta-hacking, but I'm concerned as to whether it's worthwhile continuing this endeavour. This buggy implementation already doubled the size of the plugin's main code, and I'm expecting it to grow significantly more to account for some of the TODOs. All that to prevent an error which is trivial to fix, and becomes a non-issue very quickly. It's certainly flashy and a cool example of dependency injection, but I'm not sure that the substantial increase in complexity and future maintenance efforts would be worth it.

Unless there's high demand for this or I'm craving another meta-hacking session, it's unlikely that I'll continue work on this any time soon. So if anyone wants to work further on this (or start from scratch), feel free to do so.

@Cito Cito force-pushed the main branch 16 times, most recently from 826a9b1 to 4a916ae Compare April 9, 2023 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant