Closed
Bug 1067424
Opened 10 years ago
Closed 10 years ago
Cover webapps actor 'getappActor' request with a test
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(2 files, 8 obsolete files)
4.47 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
Does this fail when bug 1037235 is applied?
Assignee | ||
Comment 3•10 years ago
|
||
Not this patch v1, but I'm working on another version, using remote frame that does reproduce it.
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
Green test - https://tbpl.mozilla.org/?tree=Try&rev=876841a707c2
Attachment #8489438 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Try on b2g desktop and emulator:
https://tbpl.mozilla.org/?tree=Try&rev=c8119fd48e76
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
I think the Mock story is much better...
Attachment #8489993 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8490867 -
Flags: review?(jryans)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
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+
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8491010 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8490867 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8491368 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8491453 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8491454 -
Flags: review+
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9e88f96e430f
https://hg.mozilla.org/mozilla-central/rev/62bec3669030
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Blocks: 998040
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•