Closed Bug 1246692 Opened 9 years ago Closed 9 years ago

Test the browser toolbox

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

I'm tired of breaking the browser toolbox without knowing about it. We desperately need a test against it. We really often break it for very dumb exception/errors that could be detected if we were just trying to open it.
I agree we should do this if possible. I know :jryans had some ideas about how to run an in-process BT that could debug everything except for devtools actors, which would be really interesting and easier to test, I'm sure.
Attached patch patch v1 (obsolete) (deleted) — Splinter Review
This is really dumb, but allowed me to see issues with my gDevTools patch. We could do something more complex by instanciating a DebuggerServer in the Browser toolbox process. There is also some opportunity with luciddream which could potentially be able to run test script immediately within the Browser toolbox process, which is the best. As it could run existing tests instead of crafting naive one like this. But given that I don't want to spend much time on this, and instead just being covered for simple startup errors, I came up with this.
Attachment #8717052 - Flags: review?(jryans)
(In reply to Brian Grinstead [:bgrins] from comment #1) > I agree we should do this if possible. I know :jryans had some ideas about > how to run an in-process BT that could debug everything except for devtools > actors, which would be really interesting and easier to test, I'm sure. I did make a tiny attempt at this, and it does work for non-debugger things... but (of course) if you stop at a breakpoint, everything hangs since the debugger UI is stopped as well. There might be something more clever we could do, like have the content process run the UI, similar in concept to the Browser Content Toolbox but backwards (in that case the parent runs the UI and connects to the child). So, still possibly viable, but not super simple.
(In reply to Brian Grinstead [:bgrins] from comment #1) > I agree we should do this if possible. I know :jryans had some ideas about > how to run an in-process BT that could debug everything except for devtools > actors, which would be really interesting and easier to test, I'm sure. Filed bug 1246715 for this.
Try run is failing. Here is a new one. Could it be that appinfo.isOfficial is true on try? I'm wondering what other check we could use?
Last try confirms that isOfficial is true on try... May be AppConstants could help? http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#38 AppConstants.MOZZILA_OFFICIAL looks like what I'm looking for.
Assignee: nobody → poirot.alex
Comment on attachment 8717052 [details] [diff] [review] patch v1 Review of attachment 8717052 [details] [diff] [review]: ----------------------------------------------------------------- I am on board with the idea, seems like it can help! isOfficial is the same as MOZILLA_OFFICIAL[1], so that's still not quite right here. I am not sure there are flags that differ between published builds and test runs...? That seems generally bad, since you'd run your tests against something different than what you deliver to users. What about this: 1. Main process has some kind of "we're testing" pref set by your test 2. ToolboxProcess.jsm already copies all its prefs into the special BT process profile[2] 3. Check that pref instead in toolbox-process-window [1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1149 [2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#184
Attachment #8717052 - Flags: review?(jryans) → feedback+
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
With a pref.
Attachment #8717980 - Flags: review?(jryans)
Attachment #8717052 - Attachment is obsolete: true
Comment on attachment 8717980 [details] [diff] [review] patch v2 Review of attachment 8717980 [details] [diff] [review]: ----------------------------------------------------------------- This is great! Thanks for looking into this. ::: devtools/client/framework/test/browser_browser_toolbox.js @@ +20,5 @@ > + let onCustomMessage = new Promise(done => { > + Services.obs.addObserver(function listener() { > + Services.obs.removeObserver(listener, "plop"); > + done(); > + }, "plop", false); Choose a more descriptive name than "plop" :P
Attachment #8717980 - Flags: review?(jryans) → review+
It would have been too easy, it failed on debug build: 09:32:00 INFO - WebConsolePanel open failed. timeout: Connection timeout. Check the Error Console on both ends for potential error messages. Reopen the Web Console to try again. 09:32:00 INFO - ************************* 09:32:00 INFO - A coding exception was thrown in a Promise resolution callback. 09:32:00 INFO - See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise 09:32:00 INFO - Full message: TypeError: panel is undefined 09:32:00 INFO - Full stack: Toolbox.prototype.loadTool/onLoad/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1272:13
Attached patch patch v3 (obsolete) (deleted) — Splinter Review
With a better event name.
Attachment #8717988 - Flags: review+
Attachment #8717980 - Attachment is obsolete: true
Could it be that the console takes more than 20s to be ready?! Would the test slave be *that slow*?!! ://
Still not green. It seems to be going further with the pref to extend the timeout. Now the test seem to complete, we see its last log message, but for some reason it still timeouts.
So It takes more than 20s to get the console to be ready. And a bit more than 50s to finish the test.
Attached patch patch v4 (deleted) — Splinter Review
Just added requestLongerTimeout and bumped the remote-timeout pref. Let's see if that pass on all platforms...
Attachment #8718268 - Flags: review+
Attachment #8717988 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: