895471 - Allow running mochitests with the browser debugger already attached

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P4)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Would be nice if we could run mochitests and have the browser debugger be attached before the test runs so we can really dive in and debug our code and use debugger statements, etc.

Comment 2

[:Gijs (he/him)]

Kludgy workaround for now:

1. Open mochitest with --no-autorun
2. Open the toolbox
3. Go to the settings pane
3a. Scroll down
4. Enable browser (+ remote?) debugging
5. Close the toolbox
6. Open a new window
7. From the new window, start the browser debugger
8. Click through the prompt
9. Close the new window
10. Run the test using the button in the mochitest window!


But yeah, it'd be so awesome if this was 1 step instead of 10.

Nick, what needs to happen to get this moving? Does the browser debugger have a commandline switch that would fire up the browser debugger on process start? If not, that's probably the first thing that needs to happen...

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I haven't actually worked on the browser debugger much, maybe Panos can provide needed info.

FWIW, I agree that a good first step would be adding a "--browser-debugger" flag to the firefox executable that opens the browser debugger as early as we can.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

There is no command line flag to start the browser debugger, but there should be! See bug 860672 for a similar work-in-progress for the browser console. We should also have the test harness set devtools.debugger.prompt-connection to false in those cases, so that no prompt needs to be clicked.

Comment 5

[:Gijs (he/him)]

Attachment: part 0: add a browser debugger flag,

I must be missing something, but I think this is what it takes on the devtools part...

Comment 6

[:Gijs (he/him)]

Attachment: part 0: add a browser debugger flag,

Per discussion on IRC, going for --jsdebugger instead.

Comment 7

[:Gijs (he/him)]

Attachment: part 1: allow passing the flag to mochitest-browser,

This is the mochitest part. I'm sure I've messed up somewhere... but it seems to work in my testing, so asking for review anyway.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 819787 [details] [diff] [review]
part 0: add a browser debugger flag,

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

This patch brings tears to my eyes, thank you!
r=me with comments addressed.

::: browser/devtools/devtools-clhandler.js
@@ +38,5 @@
>        window.focus(); // the Browser Console was already open
>      }
>  
>      if (cmdLine.state == Ci.nsICommandLine.STATE_REMOTE_AUTO) {
>        cmdLine.preventDefault = true;

cmdLine is no longer defined here, and that breaks invoking both the browser debugger and browser console simultaneously. You need to pass it as an argument to this function.

Also, I'm not 100% sure, but it looks like we would need this state check for the browser debugger case, no?

@@ +45,5 @@
>  
> +  handleDebuggerFlag: function() {
> +    let remoteDebuggingEnabled = false;
> +    try {
> +      remoteDebuggingEnabled = kDebuggerPrefs.every(function(pref) Services.prefs.getBoolPref(pref));

We are trying to move away from SpiderMonkey-specific syntax (bug 887895). Can you make this an arrow function please?

@@ +55,5 @@
> +      Cu.import("resource:///modules/devtools/DebuggerProcess.jsm");
> +      BrowserDebuggerProcess.init();
> +    } else {
> +      Cu.reportError("Could not run chrome debugger! You need the following prefs " +
> +                     "to be set to true: " + kDebuggerPrefs.join(", "));

Since this is a flag intended to be used from a terminal, how about we added a dump() as well, to make sure people won't miss this error?

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 819790 [details] [diff] [review]
part 1: allow passing the flag to mochitest-browser,

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

::: testing/mochitest/mochitest_options.py
@@ +425,5 @@
> +                "devtools.debugger.chrome-enabled=true",
> +                "devtools.chrome.enabled=true",
> +                "devtools.debugger.prompt-connection=false"
> +            ]
> +            options.autorun = False

Is this necessary? I can imagine that it would be useful to let the test start and proceed up to a point where I've placed a debugger statement that would pause execution with the browser debugger attached.

Comment 10

[:Gijs (he/him)]

Comment on attachment 819787 [details] [diff] [review]
part 0: add a browser debugger flag,

Checked in with nits fixed. Thanks for the quick review!

https://hg.mozilla.org/integration/fx-team/rev/1d5adf37d61e

Comment 11

[:Gijs (he/him)]

(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 819790 [details] [diff] [review]
> part 1: allow passing the flag to mochitest-browser,
> 
> Review of attachment 819790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mochitest/mochitest_options.py
> @@ +425,5 @@
> > +                "devtools.debugger.chrome-enabled=true",
> > +                "devtools.chrome.enabled=true",
> > +                "devtools.debugger.prompt-connection=false"
> > +            ]
> > +            options.autorun = False
> 
> Is this necessary? I can imagine that it would be useful to let the test
> start and proceed up to a point where I've placed a debugger statement that
> would pause execution with the browser debugger attached.

Hrm. I hadn't thought of this - it's certainly a good point. In fact, a next thing I was thinking of adding was a flag that would auto-branch into a debugger statement in the test assertion code if an assertion in a test (is/ok) failed. OTOH, if I don't want to recompile, it's nice if I don't have to manually put in the --no-autorun. Furthermore, because the browser debugger starts asynchronously, I'm not sure how to ensure that all the scripts have loaded into the debugger if the tests start running automatically.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #8)
> @@ +55,5 @@
> > +      Cu.import("resource:///modules/devtools/DebuggerProcess.jsm");
> > +      BrowserDebuggerProcess.init();
> > +    } else {
> > +      Cu.reportError("Could not run chrome debugger! You need the following prefs " +
> > +                     "to be set to true: " + kDebuggerPrefs.join(", "));
> 
> Since this is a flag intended to be used from a terminal, how about we added
> a dump() as well, to make sure people won't miss this error?

This would be a perfect place for DevToolsUtils.reportException[0]

[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#40

Comment 13

[:Gijs (he/him)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> This would be a perfect place for DevToolsUtils.reportException[0]
> 
> [0]
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.
> js#40

This looks like it takes an actual exception object, though... which we don't have in this case. We could throw one, but that seems unnecessary here...

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d5adf37d61e

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

There is one more patch to review and land.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 819790 [details] [diff] [review]
part 1: allow passing the flag to mochitest-browser,

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

::: testing/mochitest/mach_commands.py
@@ +184,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):

n.b.: you and bug 926338 are going to race here.

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/00a2a5ad182e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00a2a5ad182e