Closed
Bug 895471
Opened 11 years ago
Closed 11 years ago
Allow running mochitests with the browser debugger already attached
Categories
(DevTools :: Debugger, defect, P4)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 27
People
(Reporter: fitzgen, Assigned: Gijs)
References
Details
Attachments
(2 files, 1 obsolete file)
3.23 KB,
patch
|
past
:
review+
Gijs
:
checkin+
|
Details | Diff | Splinter Review |
6.92 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Priority: -- → P4
Assignee | ||
Comment 2•11 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
Reporter | ||
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
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•11 years ago
|
||
I must be missing something, but I think this is what it takes on the devtools part...
Attachment #819773 -
Flags: review?(past)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•11 years ago
|
||
Per discussion on IRC, going for --jsdebugger instead.
Attachment #819787 -
Flags: review?(past)
Assignee | ||
Updated•11 years ago
|
Attachment #819773 -
Attachment is obsolete: true
Attachment #819773 -
Flags: review?(past)
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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 9•11 years ago
|
||
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•11 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•11 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.
Reporter | ||
Comment 12•11 years ago
|
||
(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•11 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...
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment 15•11 years ago
|
||
There is one more patch to review and land.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 16•11 years ago
|
||
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•11 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 18•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•