#3823 Fix trace option for unittest test cases

[bkjchoi72]

This fixes #3823 and triggers pdb when --trace option is used for unittest test cases.

[coveralls]

Coverage decreased (-0.009%) to 93.776% when pulling 7f9b490 on bkjchoi72:3823-add-trace-option-for-unittest into a1208f5 on pytest-dev:master.

[asottile]

Looks good to me -- thanks for the patch!

[RonnyPfannschmidt]

i'll need to do a deeper review when i get a chance

[RonnyPfannschmidt]

something about this change sits very wrong with me i will require a deeper review of the interaction, cause it just looks "wrong" - it cant be right to just invoke the hook that way

[RonnyPfannschmidt]

after tracing the code for the trace plug-in and the debugging plug-in as well as the unittest plug-in, i believe this code will trigger double test execution when tracing

[bkjchoi72]

Thanks for the insight @RonnyPfannschmidt . Could you elaborate on why or how this triggers double test execution, how I can reproduce it, and possibly some ideas on how I can go about invoking the hook in a different way?

[RonnyPfannschmidt]

the double code execution happens due to in addition to running the normal test code below, when tracing calling the hook

i believe calling the hook itself is not fitting the intent of the unittest integration (as we explicitly invoke the testcase machinery of unittest for setup/teardown behaviour)

so in addition of the double test call, i believe the pyfunc_call based code additionally also skips the unittest setup/teardown machinery for the per test "scope"

to be honest without a own deeper investigation i have no idea where to actually hook in to make this "right"

[bkjchoi72]

Got it. Let me play with this over the weekend and see if I can come up with something.

[bkjchoi72]

@RonnyPfannschmidt I want to clarify the behavior of the trace plugin when the test case has a setUp() method. Let's say I have the following test case:

1 import unittest
2 class MyTest(unittest.TestCase):
3     def setUp(self):
4         assert True
5     def test_1(self):
6         assert True

When you run pytest with trace option, should this break on line 4, or line 6?

What I find interesting is that if we have a pytest function with a fixture, the break starts at the function, not the fixture. For example, let's say we have the following test:

1 import pytest
2 @pytest.fixture
3 def setup():
4     assert True
5 def test_1(setup):
6     assert True

Running pytest with trace option will break at line 6. If we want to be consistent, running the trace option on a unittest style test case should break at the actual test, not the setup. Is this the correct behavior?

[RonnyPfannschmidt]

yes, break at the start of the test is the intended behaviour

[blueyed]

I've thought the same - the hook should be triggered here, but it fails for me:

self = <_pytest.debugging.pytestPDB._init_pdb.<locals>._PdbWrapper object at 0x7f86b1850390>, func = <function Test1.test_1 at 0x7f86b1833598>, args = ()
kwds = {}, res = None

    def runcall(self, func, *args, **kwds):
        """Debug a single function call.

        Return the result of the function call.
        """
        self.reset()
        sys.settrace(self.trace_dispatch)
        res = None
        try:
>           res = func(*args, **kwds)
E           TypeError: test_1() missing 1 required positional argument: 'self'

args       = ()
func       = <function Test1.test_1 at 0x7f86b1833598>
kwds       = {}
res        = None
self       = <_pytest.debugging.pytestPDB._init_pdb.<locals>._PdbWrapper object at 0x7f86b1850390>

This is because pyfuncitem.obj is not a bound method there.

Looks like this changed then since this PR was created?

[nicoddemus]

Hi @bkjchoi72,

It has been a long time since it has last seen activity, plus we have made sweeping changes on master to drop Python 2.7 and 3.4 support, so this PR has some conflicts which require attention. You might give this another go now that master only supports Python 3.5+.

In order to clear up our queue and let us focus on the active PRs, I'm closing this PR for now.

Please don't consider this a rejection of your PR, we just want to get this out of sight until you have the time to tackle this again. If you get around to work on this in the future, please don't hesitate in re-opening this.

Thanks for your work, the team definitely appreciates it!

[blueyed]

For reference: #5996 simplifies the invocation / code around this here, but the test from this PR still fails (as expected).
Might make it easier when picking it up again though.