Closed Bug 932144 Opened 11 years ago Closed 11 years ago

Add option to break into debugger on test failure (--debug-on-failure)


(Testing :: Mochitest, defect)

Not set


(Not tracked)



(Reporter: Felipe, Assigned: Felipe)




(2 files)

Attached patch debugOnFailureSplinter Review
Add an option to break JS execution and jump to debugger when there's a testcase failure on a test run. Should be very helpful to check all the test code state at the moment that the failure happen.

This goes hand-in-hand with the --jsdebugger option from bug 895471.

And also should be extra useful to find intermittent oranges when you combine it with bug 875463 ;)

I tested it with mochitest-{plain,chrome,browser}
Attachment #823742 - Flags: review?(ted)
Attachment #823742 - Flags: review?(jmaher)
Comment on attachment 823742 [details] [diff] [review]

Review of attachment 823742 [details] [diff] [review]:

this looks good as long as you make the requirement more firm for both options --jsdebugger and --debugOnFailure.

::: testing/mochitest/
@@ +185,5 @@
>      def run_desktop_test(self, suite=None, test_file=None, debugger=None,
>          debugger_args=None, shuffle=False, keep_open=False, rerun_failures=False,
>          no_autorun=False, repeat=0, run_until_failure=False, slow=False,
> +        chunk_by_dir=0, total_chunks=None, this_chunk=None,
> +        jsdebugger=False, debug_on_failure=False):

there will be bitrot here with another patch that has startAt, endAt

::: testing/mochitest/
@@ +330,5 @@
> +        { "action": "store_true",
> +          "default": False,
> +          "dest": "debugOnFailure",
> +          "help": "breaks execution and enters the JS debugger on a test failure. Should be used together with --jsdebugger."
> +        }],

can we enforce this requirement to have a --jsdebugger option?
Attachment #823742 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #1)
> can we enforce this requirement to have a --jsdebugger option?

Without having looked at the patch, IMO this option should imply the --jsdebugger option (ie set it if it's not set, and at a point where the block that guarantees --no-autorun is set still has to run, so we get that too).
The requirement is indeed already enforced in the other added block to

> if options.debugOnFailure and not options.jsdebugger:
>   self.error("--debug-on-failure should be used together with --jsdebugger.")

I was in between adding this requirement or auto-setting of --jsdebugger, but I thought this would make the behavior more clear (i.e. easier to understand that this option is a complement to the --jsdebugger feature). But I can change that if wanted.
Attachment #823742 - Flags: review?(ted)
Attached patch patch to landSplinter Review
Thanks for the review and the bitrot heads up. Posting here the unbitrotted patch to land while the tree is closed..
Closed: 11 years ago
Resolution: --- → FIXED
Summary: Add option to break into debugger on test failure → Add option to break into debugger on test failure (--debug-on-failure)
You need to log in before you can comment on or make changes to this bug.