The default bug view has changed. See this FAQ.

Allow running mochitests with the browser debugger already attached

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Developer Tools: Debugger
P4
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: Gijs)

Tracking

Trunk
Firefox 27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Priority: -- → P4
(Assignee)

Updated

4 years ago
Duplicate of this bug: 635549
(Assignee)

Comment 2

4 years ago
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...
Flags: needinfo?(nfitzgerald)
OS: Mac OS X → All
Hardware: x86 → All
Version: 25 Branch → Trunk
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.
Flags: needinfo?(nfitzgerald) → needinfo?(past)
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.
Flags: needinfo?(past)
(Assignee)

Comment 5

4 years ago
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...
Attachment #819773 - Flags: review?(past)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
Created attachment 819787 [details] [diff] [review]
part 0: add a browser debugger flag,

Per discussion on IRC, going for --jsdebugger instead.
Attachment #819787 - Flags: review?(past)
(Assignee)

Updated

4 years ago
Attachment #819773 - Attachment is obsolete: true
Attachment #819773 - Flags: review?(past)
(Assignee)

Comment 7

4 years ago
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.
Attachment #819790 - Flags: review?(ted)
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?
Attachment #819787 - Flags: review?(past) → review+
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.
(Assignee)

Comment 10

4 years ago
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
Attachment #819787 - Flags: checkin+
(Assignee)

Comment 11

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 929535
(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
(Assignee)

Comment 13

4 years ago
(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...
https://hg.mozilla.org/mozilla-central/rev/1d5adf37d61e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
There is one more patch to review and land.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #819790 - Flags: review?(ted) → review+
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/00a2a5ad182e
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/00a2a5ad182e
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Blocks: 932144
Blocks: 933720
You need to log in before you can comment on or make changes to this bug.