Try supporting yield functions for setup/teardown.

[okken]

No description provided.

[blakeNaccarato]

I've never reviewed Pytest plugin code before, so it was good practice for me to understand how Pytest connects to things under the hood. Initially I thought some yield from and de-nested logic could help in the param_scope function, but with some local debugging I convinced myself that the way you've done it is actually the way to go.

It took me awhile to figure out that pytest_runtest_teardown is a magic function that we use to handle teardown. I can see how you need globals to handle this. Am I correct in thinking that you couldn't use a closure or inner function here because Pytest searches for pytest_runtest_teardown at module scope?

Anyways, I caught a minor copy/paste error in your newly-written test, and proposed a test (which will fail on the current state of the PR) that would check for excess teardown. Also, I think it might be better to handle the excess teardown case right away, as you're unpacking m.args, rather than handling it way down in if teardown_func:.

I appreciate the opportunity to review it, I've learned some about how to write Pytest plugins. It's more work than I thought 😅, considering you're working with lower-level constructs.

[blakeNaccarato]

Hmm, I'm not sure how to comment on un-diffed areas in a PR review! But anyways, a few lines above this where the if m: clause appears, the case of excess teardown could be handled earlier there, rather than handling it on the line comment below.

[blakeNaccarato]

Looks like one of my review comments got orphaned onto the wrong line, I think because I did it in my IDE and something went awry, so I've repeated it just now. Next time I'll just do the review in the GitHub UI instead.

Also I have a question on PR etiquette. I submitted this review as "request changes". As a visiting reviewer, could "request changes" be misconstrued as overstepping when you're not an official reviewer for that project?

[okken]

@blakeNaccarato Thanks for the feedback. I'll do a more thorough review this week.

[okken]

thanks for the great feedback

[okken]

I tried removing the teardown parameter to the maker.
But ran into an oddity with pytest where you can't send one function. Weird.
Filed pytest-dev/pytest#11519

[RonnyPfannschmidt]

@okken this plugin seems to be only necessary to work around the fact that I haven't yet managed to make function definition Part of the scopes list and collection tree

[blakeNaccarato]

So it looks pytest-dev/pytest#1552 (comment) summarizes the simplest framing of the request that this plugin attempts to satisfy, while pytest-dev/pytest#1552 (comment) and pytest-dev/pytest#1552 (comment) detail the implementation problem of how a param scope is not so simply defined, due to the fact that parametrize can be used on functions as well as fixtures, operating at various scopes? So a param scope would be orthogonal to the scope system defined as is?

With my limited understanding of pytest internals, the statement, "I haven't yet managed to make function definition part of the scopes list and collection tree," suggests that as a prerequisite feature for param scope? Because currently pytest's scope mechanism doesn't "see" functions at signature/definition time?

[RonnyPfannschmidt]

Exactly, currently there's nodes in the collection tree for anything that's contains functions and functions after parameterization

The node type for the definition was already added,but the de-spagettification off that part is a major undertaking

[okken]

@okken this plugin seems to be only necessary to work around the fact that I haven't yet managed to make function definition Part of the scopes list and collection tree

@RonnyPfannschmidt yes, that's true. When the plugin is no longer necessary, I'll happily deprecate it and point people to a recommended solution.

[blakeNaccarato]

Thanks for clarifying. Okay I'm going to cross-link a few related efforts. Looks like the groundwork was laid in https://github.com/pytest-dev/pytest/projects/1, while the node cleanup in https://github.com/pytest-dev/pytest/projects/3 will pave the way to the feature in question (also seen in pytest-dev/pytest#3432).

@okken, maybe it's a good idea to track those upstream efforts in a meta-issue, and possibly point willing contributors in that direction (as much as mere mortals are able to help with pytest internals)?

See also the related API reference on FunctionDefinition and an indication that such an effort would constitute a true breakage in the backwards compatibility policy. Wow, sometimes a seemingly innocuous feature is actually a can of worms.

[okken]

@okken, maybe it's a good idea to track those upstream efforts in a meta-issue, and possibly point willing contributors in that direction (as much as mere mortals are able to help with pytest internals)?

@blakeNaccarato I like this idea. Will you do this? :)

[blakeNaccarato]

@blakeNaccarato I like this idea. Will you do this? :)

Sure, I'll open a meta-issue in pytest-param-scope detailing the major open blockers and where additional help is needed. I will probably need to back-burner this though. I'll try to write it up this weekend.