Closed
Bug 1010387
Opened 11 years ago
Closed 10 years ago
[appmgr v2] write tests
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: paul, Assigned: paul)
References
Details
Attachments
(1 file, 8 obsolete files)
|
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
No description provided.
|
Assignee | |
Comment 1•11 years ago
|
||
Assignee: nobody → paul
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 2•11 years ago
|
||
Attachment #8422588 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 3•11 years ago
|
||
|
|
Assignee | |
Updated•11 years ago
|
Attachment #8422625 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 4•11 years ago
|
||
|
|
Assignee | |
Comment 5•10 years ago
|
||
|
|
Assignee | |
Updated•10 years ago
|
Attachment #8422980 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 6•10 years ago
|
||
Attachment #8423169 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0b46c19335f0
locales and css issues should be fixed now.
Attachment #8423791 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 8•10 years ago
|
||
Optimizer, can you help me figure out what's going on with the windows failure?
Flags: needinfo?(scrapmachines)
|
|
Assignee | |
Comment 9•10 years ago
|
||
It's probably an issue with chrome.ini.
https://tbpl.mozilla.org/?tree=Try&rev=ce6cdbcf5fe1
Attachment #8423944 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 11•10 years ago
|
||
Comment on attachment 8424031 [details] [diff] [review]
v5
Adding tests. It's not full coverage, but good enough for compiling the appmanager. I could have re-used some resources from the tests in /app-manager/, but I don't want to add more dependencies.
I'm also adding more promises. It makes writing the tests easier, but this is also required for some future work on running commands from the command line.
Attachment #8424031 -
Flags: review?(jryans)
|
|
Assignee | |
Updated•10 years ago
|
Attachment #8424031 -
Flags: review?(jryans) → review?(janx)
|
||
Comment 12•10 years ago
|
||
Comment on attachment 8424031 [details] [diff] [review]
v5
Review of attachment 8424031 [details] [diff] [review]:
-----------------------------------------------------------------
Great! It's awesome to have tests, and the code looks much better now.
The tests make sense to me, and all 33 of them worked on my machine, so r+ with a few nits (please feel free to ignore the nits you don't like).
::: browser/devtools/app-manager/app-validator.js
@@ +100,4 @@
> try {
> Services.io.newURI(manifestURL, null, null);
> } catch(e) {
> + this.error(strings.formatStringFromName("validator.invalidHostedManifestURL", [manifestURL, e.message], 2));
(this param length argument reminds me of heartbleed)
::: browser/devtools/webide/content/webide.js
@@ +544,5 @@
> +
> + if (!location) {
> + return;
> + }
> +
Nit: Trailing spaces.
::: browser/devtools/webide/modules/app-manager.js
@@ +83,4 @@
> this._listenToApps();
> this._listTabsResponse = response;
> this._getRunningApps();
> + this.update("list-tabs-response");
This looks useless.
@@ +291,4 @@
> AppManager.console.error("Can't install project. Unknown type of project.");
> return promise.reject("Can't install");
> }
> +
Nit: Trailing spaces.
::: browser/devtools/webide/test/app/index.html
@@ +2,5 @@
> +<html>
> +<head><title></title></head>
> +<body>
> +</body>
> +</html>
Note: The <html>, <head> and <body> can be omitted.
::: browser/devtools/webide/test/head.js
@@ +69,5 @@
> + }
> + deferred.resolve();
> + });
> + return deferred.promise;
> +}
Nit: Add an empty line after this one.
::: browser/devtools/webide/test/test_runtime.html
@@ +98,5 @@
> + yield win.Cmds.disconnectRuntime();
> +
> +
> + ok(win.AppManager.selectedProject, "A project is still selected");
> + ok(!isPlayActive(), "play button is disnabled 6");
Nit: s/disnabled/disabled/
Attachment #8424031 -
Flags: review?(janx) → review+
|
|
Assignee | |
Comment 13•10 years ago
|
||
Attachment #8424031 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•10 years ago
|
Attachment #8424942 -
Flags: review?(jryans)
|
|
||
Comment 14•10 years ago
|
||
Comment on attachment 8424942 [details] [diff] [review]
[appmgr v2] write tests and make good use of promises & tasks.
Review of attachment 8424942 [details] [diff] [review]:
-----------------------------------------------------------------
(Carrying over my unofficial r+, still needs Ryan's blessing.)
Attachment #8424942 -
Flags: review+
Comment on attachment 8424942 [details] [diff] [review]
[appmgr v2] write tests and make good use of promises & tasks.
Review of attachment 8424942 [details] [diff] [review]:
-----------------------------------------------------------------
Yay, tests!
Mostly nits, but I'd like to see how you clean up the "test" params.
::: browser/devtools/app-manager/app-validator.js
@@ +99,5 @@
> manifestURL = this.project.location;
> try {
> Services.io.newURI(manifestURL, null, null);
> } catch(e) {
> + this.error(strings.formatStringFromName("validator.invalidHostedManifestURL", [manifestURL, e.message], 2));
Such an odd API... Not your fault of course! Yay, XPIDL.
::: browser/devtools/webide/content/newapp.js
@@ +64,5 @@
> }
> templatelistNode.selectedIndex = 0;
> +
> + /* Chrome mochitest support */
> + let test = window.arguments[0].test;
Maybe |testApp| or |testOptions|? It's not actually the test itself.
@@ +117,5 @@
>
> + let folder;
> +
> + /* Chrome mochitest support */
> + let test = window.arguments[0].test;
Same here.
::: browser/devtools/webide/content/webide.js
@@ +16,5 @@
> const {AppProjects} = require("devtools/app-manager/app-projects");
> const {Connection} = require("devtools/client/connection-manager");
> const {AppManager} = require("devtools/app-manager");
>
> +let { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
Nit: const?
@@ +478,5 @@
> quit: function() {
> window.close();
> },
>
> + newApp: function(test) {
Maybe call this |testApp| or |testOptions| or something? It's not actually the name of the test. See other comments about this issue too.
@@ +482,5 @@
> + newApp: function(test) {
> + return UI.busyUntil(Task.spawn(function* () {
> +
> + // Open newapp.xul, which will feed ret.location
> + let ret = {location:null,test:test};
Please document what options you can pass into |test|. Though, even the the key name |test| is a bit mystifying.
Nit: Spaces after colons and commas?
@@ +530,5 @@
>
>
> + importHostedApp: function(location) {
> + return UI.busyUntil(Task.spawn(function* () {
> + let ret = {value:null};
Nit: space after colon
@@ +661,4 @@
> },
>
> disconnectRuntime: function() {
> + return UI.busyUntil(AppManager.disconnectRuntime(), "Disconnecting from runtime");
Nit: Some |busyUntil| explanation strings start with a capital letter, other don't. Pick one form. Majority seem to start lowercase for now.
@@ +669,5 @@
> return longstr.string().then(dataURL => {
> longstr.release().then(null, UI.console.error);
> UI.openInBrowser(dataURL);
> });
> + }), "Taking screenshot");
Nit: maybe lowercase, see above
@@ +762,3 @@
> break;
> }
> + return project.reject();
project -> promise?
::: browser/devtools/webide/modules/app-manager.js
@@ +82,5 @@
> this.connection.client.listTabs((response) => {
> this._listenToApps();
> this._listTabsResponse = response;
> this._getRunningApps();
> + this.update("list-tabs-response");
Where do you listen for this?
::: browser/devtools/webide/test/head.js
@@ +2,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const {utils:Cu,classes:Cc,interfaces:Ci} = Components;
Nit: spaces after colons and commas
@@ +15,5 @@
> +const {AppProjects} = require("devtools/app-manager/app-projects");
> +
> +const TEST_BASE = "chrome://mochitests/content/chrome/browser/devtools/webide/test/";
> +
> +
Nit: remove extra blank line
::: browser/devtools/webide/test/test_runtime.html
@@ +92,5 @@
> + yield nextTick();
> +
> + ok(isPlayActive(), "play button is enabled 5");
> + ok(!isStopActive(), "stop button is disabled 5");
> +
Nit: so many lines!
::: browser/devtools/webide/themes/details.css
@@ -60,5 @@
>
> #icon {
> height: 48px;
> width: 48px;
> - text-align:top;
Is this related to tests?
::: browser/devtools/webide/themes/newapp.css
@@ -42,5 @@
> richlistitem {
> -moz-box-align: start;
> }
>
> -richlistitem:nth-child(odd) {
Is this related to tests? I guess this one is just bad syntax to start with...
Attachment #8424942 -
Flags: review?(jryans)
|
|
Assignee | |
Comment 16•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (on PTO May 23 - June 4) from comment #15)
> ::: browser/devtools/webide/themes/details.css
> @@ -60,5 @@
> >
> > #icon {
> > height: 48px;
> > width: 48px;
> > - text-align:top;
>
> Is this related to tests?
>
> ::: browser/devtools/webide/themes/newapp.css
> @@ -42,5 @@
> > richlistitem {
> > -moz-box-align: start;
> > }
> >
> > -richlistitem:nth-child(odd) {
>
> Is this related to tests? I guess this one is just bad syntax to start
> with...
Strangely, yes. Got failures like:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_css.js | Got error message for jar:file:///builds/slave/talos-slave/test/build/application/FirefoxNightly.app/Contents/MacOS/browser/omni.ja!/chrome/webide/skin/newapp.css: Expected color but found '}'. Error in parsing value for 'background-color'. Declaration dropped.
|
|
Assignee | |
Comment 17•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (on PTO May 23 - June 4) from comment #15)
> ::: browser/devtools/webide/test/test_runtime.html
> @@ +92,5 @@
> > + yield nextTick();
> > +
> > + ok(isPlayActive(), "play button is enabled 5");
> > + ok(!isStopActive(), "stop button is disabled 5");
> > +
>
> Nit: so many lines!
I like it that way :)
|
|
Assignee | |
Comment 18•10 years ago
|
||
Attachment #8424942 -
Attachment is obsolete: true
Attachment #8425457 -
Flags: review?(jryans)
|
|
Assignee | |
Comment 19•10 years ago
|
||
Attachment #8425457 -
Flags: review?(jryans) → review+
|
|
Assignee | |
Updated•10 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 20•10 years ago
|
||
It requires a small rebased. I'll land it myself.
Keywords: checkin-needed
|
|
Assignee | |
Comment 21•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
|
||
Comment 22•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
|
||
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
•