Closed Bug 1067424 Opened 6 years ago Closed 6 years ago

Cover webapps actor 'getappActor' request with a test

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 8 obsolete files)

I think I finally found a way to provide a somewhat easy and "landable" way of testing the rest of the webapps actor requests we are desperatly missing in test coverage!
We should be able to catch most of our silent regression by writting a mochitest that would override the "AppFrames" variable from webapps actor with a Mock.
That wouldn't cover gaia codebase, so that we wouldn't detect brekages due to gaia regressions, but that would cover 90% of gecko regressions.
I think we were hit only once by a gaia regression, during the window manager refactoring.
Does this fail when bug 1037235 is applied?
Not this patch v1, but I'm working on another version, using remote frame that does reproduce it.
Attached patch patch v2 (obsolete) — Splinter Review
So this patch reproduces the XULAppInfo exception,
and also adds a try/catch in child.js in order to see the exception.
(No idea why the exception is trapped by loadFrameScript...)

This patch only covers getAppActor,
but we could continue and cover more.
For ex, try to send a naive request to console actor,
or check appOpen/appClose events.

https://tbpl.mozilla.org/?tree=Try&rev=177808a21dc5
Attachment #8489414 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
Green test - https://tbpl.mozilla.org/?tree=Try&rev=876841a707c2
Attachment #8489438 - Attachment is obsolete: true
Try on b2g desktop and emulator:
https://tbpl.mozilla.org/?tree=Try&rev=c8119fd48e76
Attachment #8489993 - Flags: review?(jryans)
Comment on attachment 8489993 [details] [diff] [review]
patch v3

Review of attachment 8489993 [details] [diff] [review]:
-----------------------------------------------------------------

I am so excited to finally get some coverage here! :D

Let's try to think of a better way to set the mock, but it seems great overall.

::: toolkit/devtools/apps/tests/debugger-protocol-helper.js
@@ +126,5 @@
> +addMessageListener("addFrame", function (aMessage) {
> +  let win = Services.wm.getMostRecentWindow("navigator:browser");
> +  let doc = win.document;
> +  let frame = doc.createElementNS("http://www.w3.org/1999/xhtml", "iframe");
> +  frame.setAttribute("mozbrowser", "true");

Why is this here if you can also set via the message a few lines later?  (Also, are there actually other values than "true" that make sense...?)

@@ +150,5 @@
>      gClient.close();
>    });
>  });
>  
> +

Nit: Just one blank line seems fine.

@@ +151,5 @@
>    });
>  });
>  
> +
> +DebuggerServer.AppFramesMock = {

DebuggerServer feels pretty random to me here, since it has nothing to do with loading AppFrames in the non-mock case...  But I guess I don't have any wonderful suggestions.

Maybe if the lazy load of AppFrames moved out of webapps.js to DebuggerServer for real, and then here you're replacing the real one?  That would seem less strange to me.

Or maybe there is a different way to pass the mock to the webapps actor?
Attachment #8489993 - Flags: review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] from comment #7)
> Comment on attachment 8489993 [details] [diff] [review]
> ::: toolkit/devtools/apps/tests/debugger-protocol-helper.js
> @@ +126,5 @@
> > +addMessageListener("addFrame", function (aMessage) {
> > +  let win = Services.wm.getMostRecentWindow("navigator:browser");
> > +  let doc = win.document;
> > +  let frame = doc.createElementNS("http://www.w3.org/1999/xhtml", "iframe");
> > +  frame.setAttribute("mozbrowser", "true");
> 
> Why is this here if you can also set via the message a few lines later? 
> (Also, are there actually other values than "true" that make sense...?)

Oops, we don't want to make it optional as we will only play with mozbrowser frames.
non-mozbrowser won't have any message manager and won't be debuggable.

There is no particular value for mozbrowser, it is just an attribute that has to be defined:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-mozbrowser

> @@ +151,5 @@
> >    });
> >  });
> >  
> > +
> > +DebuggerServer.AppFramesMock = {
> 
> DebuggerServer feels pretty random to me here, since it has nothing to do
> with loading AppFrames in the non-mock case...  But I guess I don't have any
> wonderful suggestions.
> 
> Maybe if the lazy load of AppFrames moved out of webapps.js to
> DebuggerServer for real, and then here you're replacing the real one?  That
> would seem less strange to me.

I'm not sure it is any better to put some random App related stuff in main.js

> 
> Or maybe there is a different way to pass the mock to the webapps actor?

A more explicit way would be to load the debugger server in a custom loader with a magic module/global.
It would mean hacking Loader.jsm with some random app related stuff...

Or expose an API on webapps.js itself to mock this object. I'll submit another patch with this approach.
So in order to ease calling webapps actor method,
I first convert it to SDK module...
That won't be harmful! Instead it should improve lazy loading of actors \o/

https://tbpl.mozilla.org/?tree=Try&rev=d84c654f668b
Attached patch patch v4 (obsolete) — Splinter Review
I think the Mock story is much better...
Attachment #8489993 - Attachment is obsolete: true
Comment on attachment 8490866 [details] [diff] [review]
Convert webapps actor to SDK module

Review of attachment 8490866 [details] [diff] [review]:
-----------------------------------------------------------------

Note that this patch depends on the just-landed bug 988237 (might still be only in fx-team)
Attachment #8490866 - Flags: review?(jryans)
Attachment #8490867 - Flags: review?(jryans)
I missed a require(Services) in webapps.js.
It is frightening that is only failed on mochitests only on b2g and none of other tests :/

https://tbpl.mozilla.org/?tree=Try&rev=584b79705bcf
Attachment #8490866 - Attachment is obsolete: true
Attachment #8490866 - Flags: review?(jryans)
Attachment #8491010 - Flags: review?(jryans)
(In reply to Alexandre Poirot [:ochameau] from comment #12)
> Created attachment 8491010 [details] [diff] [review]
> Convert webapps actor to SDK module
> 
> I missed a require(Services) in webapps.js.
> It is frightening that is only failed on mochitests only on b2g and none of
> other tests :/

Hmm, yeah, how could the XPCShell test (test_webappsActor.js) have passed?  It's only run on desktop at the moment, but still seems odd.
Comment on attachment 8491010 [details] [diff] [review]
Convert webapps actor to SDK module

Review of attachment 8491010 [details] [diff] [review]:
-----------------------------------------------------------------

Yay, more laziness!

::: toolkit/devtools/server/actors/webapps.js
@@ +17,5 @@
> +let { ActorPool } = require("devtools/server/actors/common");
> +let { DebuggerServer } = require("devtools/server/main");
> +let Services = require("Services");
> +
> +

Nit: remove extra blank line

@@ +521,5 @@
>            // frame script. That will flush the jar cache for this app and allow
>            // loading fresh updated resources if we reload its document.
>            let FlushFrameScript = function (path) {
> +            let jar = Cc["@mozilla.org/file/local;1"]
> +                                .createInstance(Ci.nsILocalFile);

Nit: re-indent

@@ +526,3 @@
>              jar.initWithPath(path);
> +            let obs = Cc["@mozilla.org/observer-service;1"]
> +                                .getService(Ci.nsIObserverService);

Nit: re-indent
Attachment #8491010 - Flags: review?(jryans) → review+
Comment on attachment 8490867 [details] [diff] [review]
patch v4

Review of attachment 8490867 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks better now!
Attachment #8490867 - Flags: review?(jryans) → review+
Attachment #8491010 - Attachment is obsolete: true
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #8490867 - Attachment is obsolete: true
Rebased.
Attachment #8491366 - Attachment is obsolete: true
Attached patch patch v5Splinter Review
Attachment #8491368 - Attachment is obsolete: true
Attachment #8491453 - Flags: review+
Attachment #8491454 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e88f96e430f
https://hg.mozilla.org/mozilla-central/rev/62bec3669030
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.