Closed Bug 1010387 Opened 7 years ago Closed 7 years ago

[appmgr v2] write tests

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(1 file, 8 obsolete files)

No description provided.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attached patch v1.1 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=cf941fc8be8e
Attachment #8422588 - Attachment is obsolete: true
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #8422625 - Attachment is obsolete: true
Blocks: 1007218
Attachment #8422980 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=893c8c69b91b
Attachment #8423169 - Attachment is obsolete: true
Depends on: 1011464
Attached patch v4 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=0b46c19335f0

locales and css issues should be fixed now.
Attachment #8423791 - Attachment is obsolete: true
Optimizer, can you help me figure out what's going on with the windows failure?
Flags: needinfo?(scrapmachines)
Attached patch v5 (obsolete) — Splinter Review
It's probably an issue with chrome.ini.

https://tbpl.mozilla.org/?tree=Try&rev=ce6cdbcf5fe1
Attachment #8423944 - Attachment is obsolete: true
that was it.aur/cortex-git
Flags: needinfo?(scrapmachines)
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)
No longer depends on: 1011464
Attachment #8424031 - Flags: review?(jryans) → review?(janx)
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+
Attachment #8424031 - Attachment is obsolete: true
Attachment #8424942 - Flags: review?(jryans)
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)
(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.
(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 :)
Attached patch patchSplinter Review
Attachment #8424942 - Attachment is obsolete: true
Attachment #8425457 - Flags: review?(jryans)
Keywords: checkin-needed
It requires a small rebased. I'll land it myself.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/58965bbf2290
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.