Last Comment Bug 895471 - Allow running mochitests with the browser debugger already attached
: Allow running mochitests with the browser debugger already attached
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
P4 normal with 1 vote (vote)
: Firefox 27
Assigned To: :Gijs
:
: Jason Laster [:jlast]
Mentors:
: 635549 (view as bug list)
Depends on: 929535
Blocks: 932144 933720
  Show dependency treegraph
 
Reported: 2013-07-18 09:27 PDT by Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
Modified: 2013-11-01 09:34 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 0: add a browser debugger flag, (3.24 KB, patch)
2013-10-21 08:33 PDT, :Gijs
no flags Details | Diff | Splinter Review
part 0: add a browser debugger flag, (3.23 KB, patch)
2013-10-21 09:08 PDT, :Gijs
past: review+
gijskruitbosch+bugs: checkin+
Details | Diff | Splinter Review
part 1: allow passing the flag to mochitest-browser, (6.92 KB, patch)
2013-10-21 09:12 PDT, :Gijs
ted: review+
Details | Diff | Splinter Review

Description User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-07-18 09:27:15 PDT
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 1 User image :Gijs 2013-09-18 01:06:04 PDT
*** Bug 635549 has been marked as a duplicate of this bug. ***
Comment 2 User image :Gijs 2013-09-18 01:09:43 PDT
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 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-09-18 10:23:22 PDT
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 User image Panos Astithas [:past] 2013-09-19 01:00:32 PDT
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 User image :Gijs 2013-10-21 08:33:50 PDT
Created attachment 819773 [details] [diff] [review]
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 User image :Gijs 2013-10-21 09:08:47 PDT
Created attachment 819787 [details] [diff] [review]
part 0: add a browser debugger flag,

Per discussion on IRC, going for --jsdebugger instead.
Comment 7 User image :Gijs 2013-10-21 09:12:56 PDT
Created attachment 819790 [details] [diff] [review]
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 User image Panos Astithas [:past] 2013-10-22 08:05:04 PDT
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 User image Panos Astithas [:past] 2013-10-22 08:27:50 PDT
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 User image :Gijs 2013-10-22 08:33:41 PDT
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 User image :Gijs 2013-10-22 08:42:26 PDT
(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 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-10-22 10:27:10 PDT
(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 User image :Gijs 2013-10-22 10:33:43 PDT
(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 User image Wes Kocher (:KWierso) 2013-10-22 14:56:03 PDT
https://hg.mozilla.org/mozilla-central/rev/1d5adf37d61e
Comment 15 User image Panos Astithas [:past] 2013-10-23 00:46:05 PDT
There is one more patch to review and land.
Comment 16 User image Ted Mielczarek [:ted.mielczarek] 2013-10-23 11:48:49 PDT
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 18 User image Carsten Book [:Tomcat] 2013-10-25 03:02:40 PDT
https://hg.mozilla.org/mozilla-central/rev/00a2a5ad182e

Note You need to log in before you can comment on or make changes to this bug.