Today, inspector tests are e10s friendly, but not remote friendly as they are still using CPOW and a frame script. The frame script abstraction looks like an actor but isn't. There isn't that much to add in order to be able to be remote friendly. We could convert the frame script to a dynamically registered actor and stop using CPOW. Then we would only need just a couple of abstraction in the test in order to connect to any arbitrary target: local tab, e10s or non-e10s one, remote app, remote tab, chrome document, ...

Here is a work in progress patch, converting just a few tests. I can already split this patch in meaningful subset, but I would like some feedback from inspector peers in order to ensure I'm sane and that this contribution will be accepted ;)

Here is the luciddream magic to make it run. I'm not really satisfied by this as it contains a list of whitelisted files to run in the Javascript helper file.

Comment on attachment 8569957 [details] [diff] [review] wip patch Review of attachment 8569957 [details] [diff] [review]: ----------------------------------------------------------------- I'm all for this. It's indeed going to be a long refactor (I know this for a fact, I've had to re-write almost all of these tests at least once), but the added value is big, and the code doesn't look more complex at all, in fact some parts of tests are even simplified a little bit. Could you rename 'actor' into something more self-explanatory? Like maybe 'pageActor' or something better. I'm worried about new contributors that only work on the front-end and haven't had to learn about what actors are yet. They'll be using this a lot when writing tests, and need to be able to know immediately what this is for when reading other tests. ::: browser/devtools/inspector/test/actor.js @@ +2,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + I'm going to assume that this file contains almost exactly the code we had in the frame-script and some of the code we had in head.js. So, that looks good to me. @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// A helper frame-script for brower/devtools/inspector tests. s/frame-script/actor ::: browser/devtools/inspector/test/browser_inspector_highlight_after_transition.js @@ +8,1 @@ > Ok that's weird, this test doesn't seem to test anything at all! Not blaming your patch, it looks like it's already the case without it. I was expecting it to test the position of the nodeinfobar, but it doesn't. @@ +19,5 @@ > +function* checkDivHeight(inspector, actor) { > + let div = yield getNodeFront("div", inspector); > + console.log(div); > + let mod = div.startModifyingAttributes(); > + mod.setAttribute("visible", "true"); I think you should add a setAttribute method on the test actor, just because it's simpler than this attribute modification batching method and because we will need to test assert changing attributes outside of the inspector code too. ::: browser/devtools/inspector/test/head.js @@ +259,2 @@ > info("Toolbox and inspector already open"); > + throw new Error("Inspector is already opened, please use getActiveInspector"); Why do you want to throw here? I seems to me that calling openInspector several times should be harmless.

Ok, so I tried to split this patch is multiple independant ones... Here is a first naive one. addTab doesn't mean anything when speaking about random remote target like apps. So I'm trying to ensure that all tests (most, as some really want to test addTab), are using openInspectorForURL.

Another really simple one. We use content.location = "http://..." to change the target document. That's not compatible with remote targets. Instead I'm now calling TabActor.navigateTo request.

Here is a try run for these two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a98274b8922

And the leftover. It is still pretty significant patch, but I don't see how to split it even more. There is still some failures, but most tests are working locally.

Fixed keybinding tests. Now I only get one failing test, locally: TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_select-last-selected.js | #id4 is selected after navigation. - Got [Front for domnode/server1.conn58.domnode76], expected [Front for domnode/server1.conn58.domnode79]

And a try with all patches to see if I get the same results on CI vs locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57f8f234f97 Patrick, do not hesitate to throw additional comments to the "wip patch", it is almost there, I'm waiting to have all tests green to request the official review. (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3) > Comment on attachment 8569957 [details] [diff] [review] > Could you rename 'actor' into something more self-explanatory? Renamed to testActor! > > ::: browser/devtools/inspector/test/actor.js > @@ +2,5 @@ > > +/* Any copyright is dedicated to the Public Domain. > > + http://creativecommons.org/publicdomain/zero/1.0/ */ > > + > > +"use strict"; > > + > > I'm going to assume that this file contains almost exactly the code we had > in the frame-script and some of the code we had in head.js. So, that looks > good to me. Yes. I tried to rename the frame script into test-actor.js in the last revision, but it failed, git still considers it as a deleted+new file :( > > @@ +19,5 @@ > > +function* checkDivHeight(inspector, actor) { > > + let div = yield getNodeFront("div", inspector); > > + console.log(div); > > + let mod = div.startModifyingAttributes(); > > + mod.setAttribute("visible", "true"); > > I think you should add a setAttribute method on the test actor, just because > it's simpler than this attribute modification batching method and because we > will need to test assert changing attributes outside of the inspector code > too. Done. > > ::: browser/devtools/inspector/test/head.js > @@ +259,2 @@ > > info("Toolbox and inspector already open"); > > + throw new Error("Inspector is already opened, please use getActiveInspector"); > > Why do you want to throw here? I seems to me that calling openInspector > several times should be harmless. Just to be more explicit, reusing an "already tested" inspector can introduce some randomness in our tests given which test is executed before... We have getActiveInspector to explicitely reuse one.

Oh try is failing as I based my patches on top of bug 1134180 and forgot to push it to try. bug 1134180 helps but I'm not sure that's a strong dependency...

(In reply to Alexandre Poirot [:ochameau] from comment #7) > Created attachment 8576770 [details] [diff] [review] > wip patch > > And the leftover. It is still pretty significant patch, but I don't see how > to split it even more. You could create a patch that only contains the test actor and its registration on the remote device, even if none of the tests use it yet. And then another (big) patch removing the duplicated functions in head.js and migrating all test files. This would make it easier to review the most important patch in this bug I guess.

Comment on attachment 8576776 [details] [diff] [review] wip patch Review of attachment 8576776 [details] [diff] [review]: ----------------------------------------------------------------- I have one request: could you measure how much time it takes to run the tests with this new setup compared to how much it takes without (locally, not with a remote device). We've had problems in the past when servers on TRY were getting slower at times and had some of our tests failing because they were taking too much time. We've had to split some tests in 2 or more parts because of this. I'd like to make sure this doesn't slow down the tests too much when running them against a local tab as today. Otherwise, as discussed earlier, I like the approach of going through an actor to access the test page. May I suggest that we start adding a comment block in each head.js file in directories that use this technique (or have a README somewhere) that explains the setup? Also, this page will need to be updated: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests Indeed, people working on the devtools for the first time may be surprised if they've been writing browser mochitests before, so it needs to be obvious why we do this, and what it is. I haven't looked at all the details in this patch yet. I have looked at head.js, test-actor.js and doc_frame_script.js changes mostly. Those changes look good to me. For the rest, it seems like boring, repetitive changes across all tests to move from local helpers or mm messages to testActor methods. So I'll look at those when you think it's ready for review. Also, TRY will be a lot better than me at finding problems for this type of changes. ::: browser/devtools/inspector/test/browser_inspector_select-last-selected.js @@ +62,1 @@ > yield selectNode(nodeToSelect, inspector); selectNode already waits for the "inspector-updated" event, so why did you need to add another step to wait for "new-node-front" here? @@ +65,4 @@ > } > > + let onNewNodeFront = inspector.selection.once("new-node-front"); > + yield navigateToAndWaitForNewRoot(toolbox, testActor, url); Same question here.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12) > May I suggest that we start adding a comment > block in each head.js file in directories that use this technique (or have a > README somewhere) that explains the setup? Yes, some kind of in-tree docs sound quite helpful for this! Maybe a docs folder with a Markdown file like we have for some server-side things already.

Comment on attachment 8569959 [details] [diff] [review] Run inspector test on luciddream I'm going to move the work of actually running the tests on luciddream to bug 1149479. I spend more time in maintaining my patch and rebasing it, rather than making it work on LD...

Rebased.

In order to help reviewing the next patch, here is one that just renames doc_frame_script.js to test-actor.js. (I don't know why, but git can't correctly handle git mv + modification correctly, it consider as a removal+new file)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a67b89f2c1c Tests are working locally. I would like to land this significant refactoring, as it is very time consuming for me to maintain this patch. Tests do not all pass on luciddream yet, so I'll most likely need to patch head.js again, but at least tests are going to start using the new test actor. About this patch, note that the TestActorFront is associated to the toolbox. So that we don't need to pass toolbox references to many methods. Also, when it comes to highlighters, I default to the toolbox default one, unless we pass as argument a specific highlighter front from which we retrieve the actorID. Overall, this patch is about replacing all usages of `content` and executeInContent with the test actor. I haven't mapped all `content` usage to a very specific actor method, sometimes I just use a magic `eval` method that allows doing a lot of very random things. We should be careful with that and only uses it for the rare and test-specific action/checks. Also, I'm considering merging all these patches into a single one before landing, the split is here to simplify the review.

I don't know why I ended up modifying markup view tests... but I shouldn't.

Comment on attachment 8586077 [details] [diff] [review] Convert addTab+openInspector to openInspectorForURL v1 Carry over

Comment on attachment 8586087 [details] [diff] [review] Convert inspector test to be remote friendly v1 Review of attachment 8586087 [details] [diff] [review]: ----------------------------------------------------------------- New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89e4c9ab7f1b Please read comment 17,18 before reviewing, if you want to test this patch you need to apply all the patches and also apply bug 1145049 before.

Comment on attachment 8586087 [details] [diff] [review] Convert inspector test to be remote friendly v1 Review of attachment 8586087 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/inspector/test/browser_inspector_highlight_after_transition.js @@ +17,4 @@ > }); > > +function* checkDivHeight(inspector, testActor) { > + yield testActor.setAttribute("div", "visible", "true"); This throws, but we are not running the test. Should I just remove it, or, (fix this test and) register it in browser.ini ? BTW if you see tests that are pointless, please shoot! Better removing them than refactoring them for nothing.

(In reply to Alexandre Poirot [:ochameau] from comment #22) > Comment on attachment 8586087 [details] [diff] [review] > Convert inspector test to be remote friendly v1 > > Review of attachment 8586087 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > browser/devtools/inspector/test/browser_inspector_highlight_after_transition. > js > @@ +17,4 @@ > > }); > > > > +function* checkDivHeight(inspector, testActor) { > > + yield testActor.setAttribute("div", "visible", "true"); > > This throws, but we are not running the test. > Should I just remove it, or, (fix this test and) register it in browser.ini ? > BTW if you see tests that are pointless, please shoot! > Better removing them than refactoring them for nothing. This test is obsolete, it used to test that the nodeinfobar would not appear outside of the content area, but now that the highlighter markup is part of the content document, this can't happen anymore. So, please remove this test.

Comment on attachment 8586087 [details] [diff] [review] Convert inspector test to be remote friendly v1 Review of attachment 8586087 [details] [diff] [review]: ----------------------------------------------------------------- That's a lot of tests to rework. Congrats for not going crazy over them. Only minor comments below, but I'd still like to see a new version before R+. ::: browser/devtools/inspector/test/browser_inspector_highlight_after_transition.js @@ +17,4 @@ > }); > > +function* checkDivHeight(inspector, testActor) { > + yield testActor.setAttribute("div", "visible", "true"); As commented earlier, please remove the test. ::: browser/devtools/inspector/test/browser_inspector_initialization.js @@ +27,5 @@ > content.document.body.innerHTML = DOCUMENT_HTML; > content.document.title = "Inspector Initialization Test"; > > + let deferred = promise.defer(); > + content.setTimeout(deferred.resolve, 2000); 2000 seems like a random time to wait here, and on particularly slow test VMs might sometimes not be enough. This is an intermittent waiting to happen. Isn't there an event we could wait instead? In any case, this at least requires a big comment saying why we need to do it. (would disabling this test for LD be an option?) ::: browser/devtools/inspector/test/browser_inspector_pseudoclass-lock.js @@ +8,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "DebuggerServer", > + "resource://gre/modules/devtools/dbg-server.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "DebuggerClient", > + "resource://gre/modules/devtools/dbg-client.jsm"); These getters don't seem like they're needed. ::: browser/devtools/inspector/test/head.js @@ +169,3 @@ > }); > > +// Register a test actor that can operate on the remote document Can you move this block of code (up until _request) to a new file in /browser/devtools/shared/ ? Maybe in a file named test-actor-registry.js or something better. And maybe move test-actor.js next to it. I'm suggesting this directory because that's where we already have frame-script-utils.js which we use in several mochitest suites. The reason why I'm asking is that so far, everytime we've added a generic test-related helper/util/function/whatever, we've added it to a head.js file, and then copy/pasted it to other head.js files over and over again. We've been discussing lately about putting them back in common in one file (see bug1147012). I'd hate to see this (rather complex) logic be copied to other head.js files when we want to run other tests on LD. You can use something like this to load this new file: Services.scriptloader.loadSubScript(testDir + "/" + filePath, this); @@ +178,5 @@ > + let { ActorRegistryFront } = require("devtools/server/actors/actor-registry"); > + let front = ActorRegistryFront(client, response); > + > + // Fetch the test actor sources > + let source = yield _request(ACTOR_URL); What about exporting request from actor-registry and adding an option to bypass the resource:// uri check instead of having to duplicate the code here? @@ +191,5 @@ > + return actorActorFront; > +}); > + > +// Load the test actor in a custom sandbox > +// as we can't use SDK module loader with URIs I have to trust you on this as I know nothing about this. But it does sound sad that we need to load the test-actor source one more time and evaluate here. Do we really not have any other way to load the module? @@ +212,5 @@ > + let client = toolbox.target.client; > + return _getTestActor(client, toolbox.target.tab, toolbox); > +}); > + > + nit: just one empty line to separate functions. @@ +236,5 @@ > + // We may have to update the form in order to get the dynamically registered > + // test actor. > + let form = yield _getUpdatedForm(client, tab); > + > + let source = yield _request(ACTOR_URL); Having to load the source once again and evaluate it is an implementation detail of _loadFront, so this line should be moved inside _loadFront. @@ +248,5 @@ > + let deferred = promise.defer(); > + try { > + uri = Services.io.newURI(uri, null, null); > + } catch (e) { > + reject(e); reject is undefined here I think. You probably wanted to call deferred.reject(e) instead. ::: browser/devtools/inspector/test/test-actor.js @@ +21,4 @@ > > +let dumpn = msg => { > + dump(msg + "\n"); > + Cu.reportError(msg); Why report an error when we're just logging a message? @@ +25,3 @@ > } > > + nit: just one empty line. @@ +54,2 @@ > > + // `selector` parameter is either just a regular selector string nit: use a jsdoc-style comment block here: /** * desc * @param ... * @return ... */ @@ +383,5 @@ > + * @param {String} selector selector identifier to select the DOM node > + * @param {String} event event name to listen to > + * @return {json} the DOM event object > + */ > + addEventListener: protocol.method(function (selector, event) { Maybe rename to addOneTimeEventListener since we're just listening once. @@ +521,5 @@ > + * @param {Object} highlighter Optional custom highlither to target > + * @return {String} value > + */ > + getSelectorHighlighterBoxNb: protocol.custom(function(highlighter) { > + return this._getSelectorHighlighterBoxNb((highlighter || this.toolbox.highlighter).actorID); The main toolbox highlighter is a BoxModelHighlighter type, it can never be a SelectorHighlighter, so for this particular method, there's no need to default to this.toolbox.highlighter if highlighter is missing, and so, no need for a custom protocol method here. @@ +557,5 @@ > + return this.getHighlighterNodeAttribute("box-model-elements", "hidden") > + .then(value => value === null); > + }, > + > + assertHighlitNode: Task.async(function* (selector) { nit: Since you're working on this, can you take the opportunity to rename this to 'assertHighlightedNode'. @@ +650,5 @@ > > + let points = polygons[0].trim().split(" ").map(i => { > + return i.replace(/M|L/, "").split(",") > + }); > + nit: trailing whitespace. @@ +758,5 @@ > + } > + > + return {d, points}; > + }) > + nit: extra empty line.

This isn't really useful to forbid using resource:// URIs, as we can easily bypass the checks by calling front._registerActor with a string fetched from a XHR or something... So I removed this check as it prevents from using ActorActor easily from tests!

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24) > Comment on attachment 8586087 [details] [diff] [review] > > That's a lot of tests to rework. Congrats for not going crazy over them. I'm especially going crazy when I get 2-3 merge to manually resolve and new test to convert, *per week* :-/ > > ::: browser/devtools/inspector/test/browser_inspector_initialization.js > @@ +27,5 @@ > > content.document.body.innerHTML = DOCUMENT_HTML; > > content.document.title = "Inspector Initialization Test"; > > > > + let deferred = promise.defer(); > > + content.setTimeout(deferred.resolve, 2000); > > 2000 seems like a random time to wait here, and on particularly slow test > VMs might sometimes not be enough. This is an intermittent waiting to > happen. Isn't there an event we could wait instead? Replaced with a executeSoon, we just need to wait a tick here. > ::: browser/devtools/inspector/test/head.js > @@ +169,3 @@ > > }); > > > > +// Register a test actor that can operate on the remote document > > Can you move this block of code (up until _request) to a new file in > /browser/devtools/shared/ ? done, in shared/tests/, test-actor.js and test-actor-registry.js > > @@ +178,5 @@ > > + let { ActorRegistryFront } = require("devtools/server/actors/actor-registry"); > > + let front = ActorRegistryFront(client, response); > > + > > + // Fetch the test actor sources > > + let source = yield _request(ACTOR_URL); > > What about exporting request from actor-registry and adding an option to > bypass the resource:// uri check instead of having to duplicate the code > here? I removed the ressouce:// uri check in a separate patch, it makes everything simplier! > > @@ +191,5 @@ > > + return actorActorFront; > > +}); > > + > > +// Load the test actor in a custom sandbox > > +// as we can't use SDK module loader with URIs > > I have to trust you on this as I know nothing about this. But it does sound > sad that we need to load the test-actor source one more time and evaluate > here. Do we really not have any other way to load the module? The SDK loader is really only about paths, we would have to hack the loader itself to be able to load test ressources. Note that ActorActor doesn't use the SDK loader because of that. May be we could something steal the front reference from ActorActor.registerActor... I haven't looked into that yet and kept the code as-is. > ::: browser/devtools/inspector/test/test-actor.js > @@ +21,4 @@ > > > > +let dumpn = msg => { > > + dump(msg + "\n"); > > + Cu.reportError(msg); > > Why report an error when we're just logging a message? It was just more handy in luciddream to see the errors in the browser console... I removed it. > @@ +521,5 @@ > > + * @param {Object} highlighter Optional custom highlither to target > > + * @return {String} value > > + */ > > + getSelectorHighlighterBoxNb: protocol.custom(function(highlighter) { > > + return this._getSelectorHighlighterBoxNb((highlighter || this.toolbox.highlighter).actorID); > > The main toolbox highlighter is a BoxModelHighlighter type, it can never be > a SelectorHighlighter, so for this particular method, there's no need to > default to this.toolbox.highlighter if highlighter is missing, and so, no > need for a custom protocol method here. Done. But I have to manually pass the actorID now. I don't know if you expected that. It means that the test now has to do: testActor.getSelectorHighlighterBoxNb(highlighterFront.actorID) > > @@ +557,5 @@ > > + return this.getHighlighterNodeAttribute("box-model-elements", "hidden") > > + .then(value => value === null); > > + }, > > + > > + assertHighlitNode: Task.async(function* (selector) { > > nit: Since you're working on this, can you take the opportunity to rename > this to 'assertHighlightedNode'. done

(In reply to Alexandre Poirot [:ochameau] from comment #27) > > @@ +521,5 @@ > > > + * @param {Object} highlighter Optional custom highlither to target > > > + * @return {String} value > > > + */ > > > + getSelectorHighlighterBoxNb: protocol.custom(function(highlighter) { > > > + return this._getSelectorHighlighterBoxNb((highlighter || this.toolbox.highlighter).actorID); > > > > The main toolbox highlighter is a BoxModelHighlighter type, it can never be > > a SelectorHighlighter, so for this particular method, there's no need to > > default to this.toolbox.highlighter if highlighter is missing, and so, no > > need for a custom protocol method here. > > Done. But I have to manually pass the actorID now. I don't know if you > expected that. > It means that the test now has to do: > testActor.getSelectorHighlighterBoxNb(highlighterFront.actorID) Ah I see, so if for consistency reason you prefer to have tests never deal with IDs, and let the front do this, then I'm fine to keep the custom front method to do this._getSelectorHighlighterBoxNb(highlighter.actorID); but we just don't need this.toolbox.highlighter in this method, that's for sure.

Comment on attachment 8589308 [details] [diff] [review] Convert inspector test to be remote friendly v2 Review of attachment 8589308 [details] [diff] [review]: ----------------------------------------------------------------- I have only looked at the most important files: test-actor.js, test-actor-registry.js and head.js, and have assumed the changes to the test files were similar to the ones I had reviewed last time (hard without a proper interdiff mechanism, btw, I've been using mozreview for a while, and their interdiff works like a charm so far, I can only encourage you to switch over to it). Anyway, those latest changes do look great to me. Thanks for fixing everything I commented about. I only have a couple of remaining comments, but other than this, r=me for this. ::: browser/devtools/shared/test/browser.ini @@ +12,5 @@ > doc_options-view.xul > head.js > leakhunt.js > + test-actor.js > + test-actor-registry.js I might be wrong, but I don't think you need to register these 2 files in this browser.ini file. ::: browser/devtools/shared/test/test-actor-registry.js @@ +18,5 @@ > +let ROOT_TEST_DIR = getRootDirectory(gTestPath); > +let ACTOR_URL = ROOT_TEST_DIR + "test-actor.js"; > + > +// Register a test actor that can operate on the remote document > +exports.registerTestActor = Task.async(function* (client) { Don't you need to import Task.jsm for this to work? @@ +29,5 @@ > + let front = ActorRegistryFront(client, response); > + > + // Then ask to register our test-actor to retrieve its front > + let actorActorFront = yield front.registerActor(ACTOR_URL, { type: { tab: true }, constructor: "TestActor", prefix: "testActor" }); > + return actorActorFront; Wow, actorActorFront! So meta.

We need to make inspector test changes (including adding a new in-page-content function) in Bug 901250. Marking this as blocking so it doesn't require another rebase, but hoping this lands asap so work there can proceed

Comment on attachment 8589307 [details] [diff] [review] Allow ressource uri for dynamically registered actors (In reply to Alexandre Poirot [:ochameau] from comment #25) > Created attachment 8589307 [details] [diff] [review] > Allow ressource uri for dynamically registered actors > > This isn't really useful to forbid using resource:// URIs, > as we can easily bypass the checks by calling front._registerActor > with a string fetched from a XHR or something... > > So I removed this check as it prevents from using ActorActor > easily from tests! Looks ok to me. Nick, I know you've appended the check at some point and I left it there. Did you have any special reason to do it? Honza

IIRC, disallowing resource URIs was done because it was a security concern, but I don't really remember the details. Passing ni on to Mark.

The intent was to make it hard(er) for untrusted content to find its way into an actor - I'm not sure whether it was possible to register actors from a string (as per comment 25) at the time. If there are ways around this, there may not be benefits in doing this.

Yes, I had a patch that worked around this check by calling the original actor method, that expects a string. We have to use a string as the child process can be remote, on another device and won't have access to chrome/resource URIs. So the client has to pipe the JS source to the server.

(In reply to Brian Grinstead [:bgrins] from comment #30) > We need to make inspector test changes (including adding a new > in-page-content function) in Bug 901250. Marking this as blocking so it > doesn't require another rebase, but hoping this lands asap so work there can > proceed Thanks a lot for waiting for my changes! It helps a lot. I'm almost there... I'm currently stuck on some issue with bug 1145049. It looks like the refactoring isn't green if I don't pull bug 1145049. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbe877e5903b But I'll try to land the green patch from bug 1145049, may be that will be enough.

(In reply to Alexandre Poirot [:ochameau] from comment #35) > But I'll try to land the green patch from bug 1145049, may be that will be > enough. Looks like it isn't enough, I got a leak reported and new other test failure if I don't take all bug patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d885d6a3c503 (this try doesn't take the InspectorActor.destroy/disconnect patch) So I think I really have to take all the patches from bug 1145049. But I don't get why browser/devtools/shared/test/browser_toolbar_tooltip.js fails with all the patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3fd9064c38 I can't easily reproduce it localy wheread it is a perma-fail on try. I'm starting to consider blacklisting this test in order to allow landing this and figuring this out in a followup.

Comment on attachment 8590505 [details] [diff] [review] Fix browser_toolbar_webconsole_errors_count when run independently Review of attachment 8590505 [details] [diff] [review]: ----------------------------------------------------------------- Please add a note to the commit message about how DeveloperToolbar.show returns a promise

Comment on attachment 8590507 [details] [diff] [review] Fix exception when using SimpleTest.expectUncaughtException Wrong bug.

New try, with new patches, hopefully green now: https://hg.mozilla.org/try/pushloghtml?changeset=1744b443412d

(In reply to Alexandre Poirot [:ochameau] from comment #42) > New try, with new patches, hopefully green now: > https://hg.mozilla.org/try/pushloghtml?changeset=1744b443412d Looks like there is just one more.. a leak in inspector/test/browser_inspector_highlighter-by-type.js: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1744b443412d

(In reply to Brian Grinstead [:bgrins] from comment #43) > (In reply to Alexandre Poirot [:ochameau] from comment #42) > > New try, with new patches, hopefully green now: > > https://hg.mozilla.org/try/pushloghtml?changeset=1744b443412d > > Looks like there is just one more.. a leak in > inspector/test/browser_inspector_highlighter-by-type.js: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1744b443412d Finally identified the leak and many others... I'll attach yet another new patch on the pile shortly.

Rebased.

This leaks comes from a patch in bug 1145049. Here is a new try push with updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f274933ca9c

Crazy! When I fix leaks, tbpl reports me new ones...

I finally got a green try with all the patches! https://treeherder.mozilla.org/#/jobs?repo=try&revision=7737aa9f7b26 But here is another one, I had to rebase the refactoring patch again this morning... https://treeherder.mozilla.org/#/jobs?repo=try&revision=1352523e3b98

Rebased.

New leaks identified in styles.js, yet another new fix and new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de24e28b692d

Comment on attachment 8592136 [details] [diff] [review] Convert inspector test to be remote friendly v3 Try is green!

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90fc5e34d026 Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/145f6347bb11 https://hg.mozilla.org/integration/fx-team/rev/120d272971a6 https://hg.mozilla.org/integration/fx-team/rev/998c4e53c1f8 https://hg.mozilla.org/integration/fx-team/rev/3570dbae06e2

backd this out since the depending bug bug 1145049 was backed out

New try with a simplier patch to cleanup inspector actor: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a0f3d3286a4 Let's see if that's enough to make the new remote test to pass.

I'll try to at least flush the patch that don't break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a22fd59097c

Bug 1155168 seems to help, at least it fixes the leak locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b204731a312a

Just landed attachment 8591632 [details] [diff] [review], that to unlock bug 1155168, that itself should allow landid the other patches in this bug...

If there are techniques here we could factor out into the luciddream harness to make writing these sorts of tests easier I would be all for that.

Oops should have set leave-open... Still miss some patches to land.

Just some rebase.

Comment on attachment 8650452 [details] [diff] [review] Convert addTab+openInspector to openInspectorForURL v3 Carrying over review after rebase for these two simple patches.

I had to patch this test in order to prevent a race on e10s. Markup-view call showBrieflyBoxModel, which spawn showBoxModel/hideBoxModel requests. And that doesn't delay `inspector-updated` event. See onNewSelection. The test correctly waits for inspector-updated, we should ensure that this event is fired *after* the requests are all done. In this patch, I took care of note delaying the call to showNode() by waiting on showBrieflyBoxModel. Instead both run in parallel.

Comment on attachment 8650454 [details] [diff] [review] Convert inspector test to be remote friendly v4 You already r+ this patch, and this is just a rebase. But this one is not naive and I had to resolve merge conflict since April... Feel free to take a look at other rebased patches as well. This is now green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08bb43bf4a51 (with the additional fix I just asked for review) (Note that this is based on top of bug 1195825 if you want to apply it.) Also, this refactoring isn't enough to make all tests to work on luciddream, I'll most likely need another refactoring step (or a lot of random fixes) to get them all up on running there.

Comment on attachment 8650457 [details] [diff] [review] Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js Review of attachment 8650457 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks Alex for cleaning this up!

Comment on attachment 8650454 [details] [diff] [review] Convert inspector test to be remote friendly v4 Review of attachment 8650454 [details] [diff] [review]: ----------------------------------------------------------------- I've not spent a huge amount of time reviewing this rebased patch to be honest. I had already R+'d it and looked at it thoroughly in the past. And I'm not seeing a lot of differences in this new version, so I'm going to R+ it again. ::: browser/devtools/shared/test/test-actor-registry.js @@ +48,5 @@ > + Cu.evalInSandbox(sourceText, sandbox, "1.8", ACTOR_URL, 1); > + return sandbox.exports; > +}); > + > +let _getUpdatedForm = function (client, tab) { Just a remark, you don't have to do this: since we're explicitly exporting public members on 'exports', I don't see a big reason to prefix private members with an underscore.

Comment on attachment 8650457 [details] [diff] [review] Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js Moving this patch to bug 1195825 as this race seems to be related to promise change rather than the refactoring.

Removed `_` prefixes.

All green but a leak from dependency bug 1195825.

https://hg.mozilla.org/integration/fx-team/rev/f72b3cb19b5d https://hg.mozilla.org/integration/fx-team/rev/0c0b99881870 https://hg.mozilla.org/integration/fx-team/rev/06e01800d4fe

https://hg.mozilla.org/mozilla-central/rev/f72b3cb19b5d https://hg.mozilla.org/mozilla-central/rev/0c0b99881870 https://hg.mozilla.org/mozilla-central/rev/06e01800d4fe

\o/ Now, the tests still don't run correctly on luciddream, but at least they theoretically can. Some already pass, may we can start running some.