Closed Bug 1025125 Opened 6 years ago Closed 5 years ago

If enabled, enter pdb session when test fails

Categories

(Testing :: Marionette, defect, P2)

defect

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: mdas, Assigned: parkouss)

Details

(Keywords: pi-marionette-runner, Whiteboard: [affects=loop webqa])

Attachments

(1 file, 2 obsolete files)

If we're debugging a test, we usually add 'import pdb;pdb.set_trace()' at the point we know will fail, which isn't very elegant since it requires knowledge of the failure is, and we had to run the test at least once to know where that point it. Dmose has a better idea, which was to automatically drop you into a pdb session if your test runs into an unexpected event.

We can enable this feature via a cli flag, like --debug.
Priority: -- → P2
I would propose further that we could support best practices (don't let your tests, remove friction from the code/build/debug cycle) by defaulting --debug default to on, and the build automation scriptage would explicitly disable it when it's undesirable.
Defaulting --debug to on would also make it easier to debug intermittent failures, since they can occur only rarely, and not when you're explicitly looking for them.
Adding --debug sounds like a good idea. When exception occurs, it does not print log until all the test cases are run in that file. It would be useful to drop in a debug session immediately to find the failure. 

It might be even better to support customized Python debugger like --debugger. I personally use "import ipdb;ipdb.set_trace()" for example.
Note: added [affects=loop webqa] since it will help their productivity
Definitely; thanks!
How we did it in Talkilla (which wasn't quite as automated as I remembered, but possibly still useful), is described (near the bottom of this section) in 

https://github.com/mozilla/talkilla/blob/master/docs/tests.md#functional-tests

The debug_on function is defined here:

https://github.com/mozilla/talkilla/blob/master/test/functional/browser_test.py#L15

We were using ipdb instead of pdb, but I suspect there's no interesting API difference here.
Ok, I'd like to look into this - AutomatedTester gave me the link :)
Assignee: nobody → j.parkouss
Julien: thanks; this will be super helpful!
So I added two options:

--debugger to enable the interactive python debugger
--debugger-name to be able to specify another debugger (like ipdb)

These options are available from the mach command too. I tested that a bit, and it seems to work fine.
Attachment #8574778 - Flags: review?(ahalberstadt)
Comment on attachment 8574778 [details] [diff] [review]
If enabled, enter pdb session when test fails

Review of attachment 8574778 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good but a few minor things to move around.

::: testing/marionette/client/marionette/marionette_test.py
@@ +208,5 @@
> +
> +    :param debugger_name: can be specified to use another debugger, like
> +                          ipdb. Default is to use the standard pdb.
> +    """
> +    traceback = sys.exc_info()[2]

sys.last_traceback is a bit more readable.

@@ +215,5 @@
> +        try:
> +            debugger = __import__(debugger_name)
> +        except ImportError:
> +            sys.stderr.write("Unable to load the debugger %s. Defaulting to"
> +                             " pdb.\n" % debugger_name)

Let's move this check over to the command line validation so that a user doesn't have to wait for a test failure before finding out that they mistyped the debugger name. While we're doing the import next to the command line anyway, might as well just pass in the actual debugger object into CommonTestCase and merge this function with self._enter_pm.

@@ +226,5 @@
>      __metaclass__ = MetaParameterized
>      match_re = None
>      failureException = AssertionError
> +    use_debugger = False
> +    debugger_name = None

See above, these could just be self.debugger.

::: testing/marionette/client/marionette/runner/base.py
@@ +402,5 @@
>                          dest='jsdebugger',
>                          action='store_true',
>                          default=False,
>                          help='Enable the jsdebugger for marionette javascript.')
> +        self.add_option('--debugger',

Maybe call this --pydebugger to follow the --jsdebugger convention above and reduce confusion between the two?

@@ +409,5 @@
> +                        default=False,
> +                        help='Enable python post-mortem debugger when a test fails.')
> +        self.add_option('--debugger-name',
> +                        dest='debugger_name',
> +                        help='Choose a python debugger (like ipdb). Defaults to pdb.')

Is there a way to allow both '--foo' and '--foo=bar' in optparse? E.g with argparse we could do:
parser.add_argument('--debugger', nargs='?', const='pdb')

But that doesn't seem to work with optparse :(.

::: testing/marionette/mach_commands.py
@@ +125,5 @@
>          help='Enable the jsdebugger for marionette javascript.')
> +    @CommandArgument('--debugger', action='store_true',
> +        help='Enable python post-mortem debugger when a test fails.')
> +    @CommandArgument('--debugger-name',
> +        help='Choose a python debugger (like ipdb). Defaults to pdb.')

We could potentially do that argparse hack here and remove the second option from the harness.. I'm possibly overthinking this though :).
Attachment #8574778 - Flags: review?(ahalberstadt) → review-
> Maybe call this --pydebugger to follow the --jsdebugger convention above and
> reduce confusion between the two?

Oh, :AutomatedTested asked to call that --debugger. But yeah, --py-debugger seems fine. :)

> Is there a way to allow both '--foo' and '--foo=bar' in optparse? E.g with
> argparse we could do:
> parser.add_argument('--debugger', nargs='?', const='pdb')
> 
> But that doesn't seem to work with optparse :(.

No, that does not works, the doc is clear about that in this paragraph: https://docs.python.org/2/library/optparse.html#terminology.

> ::: testing/marionette/mach_commands.py
> @@ +125,5 @@
> >          help='Enable the jsdebugger for marionette javascript.')
> > +    @CommandArgument('--debugger', action='store_true',
> > +        help='Enable python post-mortem debugger when a test fails.')
> > +    @CommandArgument('--debugger-name',
> > +        help='Choose a python debugger (like ipdb). Defaults to pdb.')
> 
> We could potentially do that argparse hack here and remove the second option
> from the harness.. I'm possibly overthinking this though :).

So, what should I do ? Let the two options ? Use one with nargs in argparse and not provide the option through mach ? Provide an environment variable to be able to run this from mach ?

I talked with :dmose on irc and he really would like to be able to specify the debugger as far as I understood.
Ok, after some discussion on irc, we endend up in a --pydebugger option that requires an argument, eg:

./mach marionette-test --pydebugger pdb

So ipdb (or whatever has the same interface) can be used also.
Attachment #8574778 - Attachment is obsolete: true
Attachment #8575293 - Flags: review?(ahalberstadt)
Comment on attachment 8575293 [details] [diff] [review]
If enabled, enter pdb session when test fails

Review of attachment 8575293 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r+ with one change.

::: testing/marionette/client/marionette/runtests.py
@@ +19,5 @@
> +        try:
> +            pydebugger = __import__(options.pydebugger)
> +        except ImportError:
> +            sys.stderr.write("Unable to load the debugger %s. Defaulting to"
> +                             " pdb.\n" % options.pydebugger)

I think it would be better to just let this error out. Likely the user just mistyped or forgot to activate a virtualenv and might get confused when they end up in pdb instead of whatever they were trying for.
Attachment #8575293 - Flags: review?(ahalberstadt) → review+
Ok, I removed the exception handling there - this will crash with an import error at the beginning if the --pydebugger passed can not be loaded.

Carrying myself the r+ from the last review.
Attachment #8575293 - Attachment is obsolete: true
Attachment #8575306 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5ffdbb39bab8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.