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

Fix --trace for parametrized tests #6099

Merged
merged 1 commit into from Nov 1, 2019

Conversation

@davidszotten
Copy link
Contributor

davidszotten commented Oct 29, 2019

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

@davidszotten davidszotten force-pushed the davidszotten:trace_parametrize branch from d3372f4 to 96d8d0c Oct 29, 2019
Copy link
Member

RonnyPfannschmidt left a comment

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

src/_pytest/debugging.py Outdated Show resolved Hide resolved
testing/test_pdb.py Outdated Show resolved Hide resolved
src/_pytest/debugging.py Outdated Show resolved Hide resolved
@davidszotten davidszotten force-pushed the davidszotten:trace_parametrize branch from c9a4d53 to b421aef Oct 29, 2019
@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 29, 2019

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

Copy link
Member

RonnyPfannschmidt left a comment

Thanks for the swift follow ups,
great work

@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 29, 2019

Thank you for the quick reviews!

@blueyed blueyed force-pushed the davidszotten:trace_parametrize branch from b421aef to e2b6ab0 Oct 30, 2019
Copy link
Contributor

blueyed left a comment

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

@blueyed blueyed force-pushed the davidszotten:trace_parametrize branch from e2b6ab0 to 3844cec Oct 30, 2019
@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

Also amended the changelog (punctuation).

@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

Thanks @blueyed

# that needs to move to FunctionDefinition
new_list = list(pyfuncitem._fixtureinfo.argnames)
new_list.append("func")
pyfuncitem._fixtureinfo.argnames = tuple(new_list)

This comment has been minimized.

Copy link
@blueyed

blueyed Oct 30, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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

@blueyed blueyed mentioned this pull request Oct 30, 2019
@blueyed blueyed force-pushed the davidszotten:trace_parametrize branch from 4912ed9 to 94ac6bf Oct 30, 2019
@blueyed blueyed dismissed their stale review Oct 30, 2019

dismissing for now, since it changed (using functools)

@blueyed blueyed requested a review from RonnyPfannschmidt Oct 30, 2019
new_list.append("func")
pyfuncitem._fixtureinfo.argnames = tuple(new_list)
pyfuncitem.obj = functools.partial(_pdb.runcall, testfunction)
pyfuncitem.obj.__name__ = testfunction.__name__

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Oct 30, 2019

Member

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

This comment has been minimized.

Copy link
@blueyed

blueyed Oct 30, 2019

Contributor

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?

This comment has been minimized.

Copy link
@blueyed

blueyed Oct 30, 2019

Contributor

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.

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Oct 30, 2019

Member

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

This comment has been minimized.

Copy link
@blueyed

blueyed Oct 30, 2019

Contributor

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.

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Oct 30, 2019

Member

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

This comment has been minimized.

Copy link
@davidszotten

davidszotten Oct 30, 2019

Author Contributor

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"

@blueyed blueyed force-pushed the davidszotten:trace_parametrize branch 2 times, most recently from c45761c to b421aef Oct 30, 2019
@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

@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

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

@davidszotten davidszotten force-pushed the davidszotten:trace_parametrize branch from b421aef to 844a957 Oct 30, 2019
@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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:
2019-10-30-224836_745x136_escrotum

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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

@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

Use co-authored-by?

ah, hadn't heard of that before

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 31, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 31, 2019

@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

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Oct 31, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 31, 2019

@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 comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Oct 31, 2019

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

@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 31, 2019

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

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Oct 31, 2019

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

@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 31, 2019

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

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

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!`

Implementation idea, test (and changelog tweaks) thanks to blueyed

Co-Authored-By: Ronny Pfannschmidt <opensource@ronnypfannschmidt.de>
Co-Authored-By: Daniel Hahler <git@thequod.de>
@davidszotten davidszotten force-pushed the davidszotten:trace_parametrize branch from 1463321 to 285524c Oct 31, 2019
@davidszotten

This comment has been minimized.

Copy link
Contributor Author

davidszotten commented Oct 31, 2019

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

@blueyed
blueyed approved these changes Nov 1, 2019
@blueyed blueyed merged commit dc5a4fb into pytest-dev:master Nov 1, 2019
5 checks passed
5 checks passed
codecov/changes No unexpected coverage changes found.
Details
codecov/patch 100% of diff hit (target 96.35%)
Details
codecov/project 96.36% (+<.01%) compared to cefe6bf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pytest-CI #20191031.5 succeeded
Details
@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Nov 1, 2019

Thanks! :)

bors bot added a commit to duckinator/bork that referenced this pull request Nov 17, 2019
Merge #76
76: Update pytest to 5.2.4 r=duckinator a=pyup-bot


This PR updates [pytest](https://pypi.org/project/pytest) from **5.2.2** to **5.2.4**.



<details>
  <summary>Changelog</summary>
  
  
   ### 5.2.4
   ```
   =========================

Bug Fixes
---------

- `6194 &lt;https://github.com/pytest-dev/pytest/issues/6194&gt;`_: Fix incorrect discovery of non-test ``__init__.py`` files.


- `6197 &lt;https://github.com/pytest-dev/pytest/issues/6197&gt;`_: Revert &quot;The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.&quot;.
   ```
   
  
  
   ### 5.2.3
   ```
   =========================

Bug Fixes
---------

- `5830 &lt;https://github.com/pytest-dev/pytest/issues/5830&gt;`_: The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.


- `6099 &lt;https://github.com/pytest-dev/pytest/issues/6099&gt;`_: Fix ``--trace`` when used with parametrized functions.


- `6183 &lt;https://github.com/pytest-dev/pytest/issues/6183&gt;`_: Using ``request`` as a parameter name in ``pytest.mark.parametrize`` now produces a more
  user-friendly error.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Homepage: https://docs.pytest.org/en/latest/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
bors bot added a commit to rehandalal/therapist that referenced this pull request Nov 19, 2019
Merge #103
103: Update pytest to 5.2.4 r=rehandalal a=pyup-bot


This PR updates [pytest](https://pypi.org/project/pytest) from **5.2.2** to **5.2.4**.



<details>
  <summary>Changelog</summary>
  
  
   ### 5.2.4
   ```
   =========================

Bug Fixes
---------

- `6194 &lt;https://github.com/pytest-dev/pytest/issues/6194&gt;`_: Fix incorrect discovery of non-test ``__init__.py`` files.


- `6197 &lt;https://github.com/pytest-dev/pytest/issues/6197&gt;`_: Revert &quot;The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.&quot;.
   ```
   
  
  
   ### 5.2.3
   ```
   =========================

Bug Fixes
---------

- `5830 &lt;https://github.com/pytest-dev/pytest/issues/5830&gt;`_: The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.


- `6099 &lt;https://github.com/pytest-dev/pytest/issues/6099&gt;`_: Fix ``--trace`` when used with parametrized functions.


- `6183 &lt;https://github.com/pytest-dev/pytest/issues/6183&gt;`_: Using ``request`` as a parameter name in ``pytest.mark.parametrize`` now produces a more
  user-friendly error.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Homepage: https://docs.pytest.org/en/latest/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.