Fix --trace for parametrized tests

[davidszotten]

I'm not sure if this is the correct / best way to fix so please check
critically when reviewing

Without this, the second time it tries to stop in a parametrized
function it raises instead:

ValueError: --trace can't be used with a fixture named func!

Also lift the restriction that was disallowing --trace on tests with a parameter named func

[RonnyPfannschmidt]

great work in finding a temporary solution that sorts the issue for now

as its a necessary kludge, i proposed 2 changes to help make it more clear

[davidszotten]

ah, apologies i misread the instructions. while updating i also squashed the 3 commits, hope that's ok

[RonnyPfannschmidt]

Thanks for the swift follow ups,
great work

[davidszotten]

Thank you for the quick reviews!

[blueyed]

Thanks!
I've hardened the test a bit (and force-pushed, old head: b421aef).

[blueyed]

Also amended the changelog (punctuation).

[davidszotten]

Thanks @blueyed

[blueyed]

The false branch for "if "func" not in pyfuncitem._fixtureinfo.argnames:" is not covered.
@RonnyPfannschmidt
Is that something that can happen?
Can/should we cover it then?

[blueyed]

I've pushed a fixup to use functools.partial there.

dismissing for now, since it changed (using functools)

[RonnyPfannschmidt]

that solution checks out proper,
nonetheless this feels like a hijacking of the pr to me

[blueyed]

nonetheless this feels like a hijacking of the pr to me

why?
If it wasn't wanted then you could uncheck the box that allows it.
It can always be reset also (I've explicitly mentioned the old head).
I'm just trying to help fixing the issue (and limitations that were in place due to unknown reasons).

Why shouldn't I push hardening/improvements when I have them locally already, so that they can be reviewed/tested on CI?

@davidszotten
What do you think?

[blueyed]

Reset it to b421aef (from c45761c). The test was timing out on CI, and I do not want to leave the impression to hijack / being hostile.
Will try to fix this in my fork, and then leave a comment here.

[RonnyPfannschmidt]

the human interaction perception of such actions of merit can widely vary,
whiles some may be accepting of that, for me personally it would be the last time that i'd contribute to the project

we should perhaps clarify on what we hope to accomplish by enabling maintainers to push commits to other prs

for me the main reason is to help new contributors that struggle with the tooling,
for better solutions/code change suggestions outside of pre-commit,
i would always strongly suggest to use something like proposed edits for guidance

for me there is a very important difference between guiding/mentoring and accidentally taking over

and i perceive your actions as "taking over" instead of guiding/mentoring

[blueyed]

for me personally it would be the last time that i'd contribute to the project

why?
It's not like anything has been merged here already, it's just still being worked on. References are/were provided to revert/reset it, and IMHO it is far better to have CI results and an actual commit rather than a diff in a comment. But I will stick to the latter then in the future here.

strongly suggest to use something like proposed edits for guidance

I'm doing that also, but here it was not fitting. Posting a diff then maybe instead.

[RonnyPfannschmidt]

actions demonstrate values and projects that brush over contributors/new contributors don't foster/respect them, and i don't want that kind of interaction

[davidszotten]

agreed. though as feedback to you @RonnyPfannschmidt , of course @blueyed is also a contributor and maybe we can think of a better way to explain the behaviours we want to promote and why than something as brief as "this feels like a hijacking of the pr"

[davidszotten]

i don't feel that strongly and can see both sides of this argument. i'm not sure i know enough about the internals to tell how "controversial" your new implementation is. but if that needs more discussion then that does feel like maybe it could be split out into a separate pr and leaving this pr to just tweak the existing workaround to work better

on the other hand, if this is just obviously better and fixes the problem, maybe it's a small enough change that it's better to just bundle this together

on the contributor management side, i'm relatively seasoned in open source and experienced enough to not mind, but someone more new to open source or this project might find it daunting and/or off-putting to have their pr "hijacked" (have someone else force-push a whole new / different implementation without talking it over first)

[blueyed]

Thanks for clarifying - it wasn't my intention to step on toes here.
It is reset, so we're back to where I can suggest to using functools here maybe.. :)

have their pr "hijacked" (have someone else force-push a whole new / different implementation without talking it over first)

FWIW, it was a separate commit on top. The previous force-push was for the test hardening and typo/changelog fix (I do not think that merits extra commits, and we cannot squash-merge (#4361)).

[RonnyPfannschmidt]

i also agree that the proposed/new solution by @blueyed is a better solution

however with the "maintainer/project hat on" i had to make a clear mention about the maintainer interaction behaviour here is potentially problematic

i'm happy that @davidszotten is on the accepting side and would like to see the proposed solution go into the final thing in a collaborative manner

[davidszotten]

ah, i missed that the impl change was a separate commit. however i think the comments about people feel like their prs are taken over would almost equally apply with new commits added to their pr.

as for multiple commits, i think it's easier to have multiple commits while in wip, and then (e.g.) ask the pr owner to squash once it's done

re: the actual impl (which i'm happy to force-push onto this myself). did you figure out why the tests hang?

[davidszotten]

@blueyed hm from what i can see, even with the impl based on partial, --trace is unhappy with a param named func (easier to see by running manually instead of inside pexpect)

[RonnyPfannschmidt]

ah, it needs a different wrapper, the partial has the positional argument named func already used up,

so it cant have that as fixture anymore

the solution is to create a wrapper with a complete own signature and invoke the running inside

[davidszotten]

actually, the issue is that the implementation of p(well, b)db.runcall "uses" a param named func for itself. i think the solution is to lift out its implementation. pr update coming soon

[blueyed]

Ah.. in 3.8 it is positional-only (https://github.com/python/cpython/blob/9e87d5d964f4f49c24dc3dd4bb7e84ec8db1876e/Lib/bdb.py#L621).

I'm not sure why we need to support kwargs named "func" for the param, when pytest does not allow for them to be kwargs in the first place? ("function already takes an argument 'param' with a default value")

import pytest


@pytest.mark.parametrize("param", [1, 2])
def test_func_kw(param="foo"):
    pass

[blueyed]

@blueyed hm from what i can see, even with the impl based on partial, --trace is unhappy with a param named func (easier to see by running manually instead of inside pexpect)

This passes/passed? (on c45761c)

@pytest.mark.parametrize('func', [1,2])
def test_func(func, request):
    assert func in (1, 2)
    assert request.function.__name__ == "test_func"

Also, btw, please do not mention me in commit messages via @blueyed (triggers notifications, also later when being "merged around").

[davidszotten]

Also, btw, please do not mention me in commit messages via @blueyed (triggers notifications, also later when being "merged around").

ah, apologies. would you like me to edit that out? if so, can i keep your name without the @ for credit?

(not sure i understand what you mean by "merged around")

[davidszotten]

maybe the partial solution needs 3.8+ to work (sorry a little hard to tell the order of your two comments so maybe this is already what you were saying)

[blueyed]

ah, apologies. would you like me to edit that out? if so, can i keep your name without the @ for credit?

Use co-authored-by?

(not sure i understand what you mean by "merged around")

Every time the commit is used, also here already:

[blueyed]

maybe the partial solution needs 3.8+ to work (sorry a little hard to tell the order of your two comments so maybe this is already what you were saying)

No, I've tested that with 3.7.4. But might be different with older versions even more.

[blueyed]

Does #6099 (comment) not work for you (using partial with func as arg)? Which Python then?

[davidszotten]

python -V
Python 3.7.0
(pytest)david@odie:pytest (trace_parametrize)(2)$ cat p.py
import pdb

def foo(func):
    pass

pdb.runcall(foo, func=None)
(pytest)david@odie:pytest (trace_parametrize)(2)$ python p.py
Traceback (most recent call last):
  File "p.py", line 6, in <module>
    pdb.runcall(foo, func=None)
  File "/Users/david/.pyenv/versions/3.7.0/lib/python3.7/pdb.py", line 1600, in runcall
    return Pdb().runcall(*args, **kwds)
TypeError: runcall() got multiple values for argument 'func'

[davidszotten]

Use co-authored-by?

ah, hadn't heard of that before

[blueyed]

Use co-authored-by?

ah, hadn't heard of that before

You have it for ronny.. ;)
(GitHub might have added it)

pdb.runcall(foo, func=None)

Sure.
But the functools.partial solution does not use that, but pdb.runcall(func), doesn't it?
(pyfuncitem.obj = functools.partial(_pdb.runcall, testfunction), not func=testfunction)

It does not work when the original function as a kwarg "func", but like I've said pytest dues not support this for fixtures anyway.
All of these pass for me:

import pytest


@pytest.mark.parametrize('myparam', [1, 2])
def test_1(myparam, request):
    assert myparam in (1, 2)
    assert request.function.__name__ == "test_1"


@pytest.mark.parametrize('func', [1, 2])
def test_func(func, request):
    assert func in (1, 2)
    assert request.function.__name__ == "test_func"


@pytest.mark.parametrize('myparam', [1, 2])
def test_func_kw(request, myparam, func="foo"):
    assert myparam in (1, 2)
    assert func == "foo"
    assert request.function.__name__ == "test_func_kw"


# pytest fails here with:
# > function already takes an argument 'func' with a default value
# @pytest.mark.parametrize('func', [1, 2])
# def test_func_kw2(request, func="foo"):
#     pass

What combination am I missing?

[davidszotten]

for me, the partial implementation fails on your second test case (with --trace, raising TypeError: runcall() got multiple values for argument 'func') (it passes without --trace)

[davidszotten]

the signature of the original runcall is def runcall(self, func, *args, **kwds): which means it can't handle kwds containing func (as my pdb.runcall example shows)

[blueyed]

It's fixed here: python/cpython@a37f356#diff-5c870dcafc8a80ef16e789bb8e901de0.
But yeah, needs wrapping then.
You could use the same method though, instead of copying the internals / function body.

[blueyed]

But might be ok to just bail out for "func" on TypeErrors (like before).
I.e. it will be lean and clean, and will support "func" with 3.7.4+ automatically.

[davidszotten]

not sure i understand what you mean by

You could use the same method though, instead of copying the internals / function body

could you explain please?

[davidszotten]

@RonnyPfannschmidt do you have a preference between a more complicated implementation to support params named func on py < 3.7.4 vs just bailing out as we do now?

[RonnyPfannschmidt]

I'm on mobile, the trick is to make a new function that shares the original signature and invokes run all with a partial of the original function and it's arguments as the sole parameters

[davidszotten]

@RonnyPfannschmidt my understanding is that this is not possible (on py < 3.7.4), as per #6099 (comment)

because runcall takes the function to call as a param named func, func may not appear in the kwargs (regardless if how it's called). that's why we need to bypass/reimplement it .

(def runcall(self, func, *args, **kwds):)

[RonnyPfannschmidt]

This is why I am talking about creating a real wrapper that passes only a func into runcall

[davidszotten]

Oh I see what you mean. But won’t that mean pdb will start in the wrapper? That might be confusing

[RonnyPfannschmidt]

No, runcall would be passed a partial as func on test call

[davidszotten]

No, runcall would be passed a partial as func on test call

ok now i'm confused again. could you send some (pseudo/)code?

[davidszotten]

ok think i finally understand. pushed an update. way better, thanks!

[blueyed]

Thanks! :)