Merge lp:~bcsaller/juju-gui/phantom-testing into lp:juju-gui/experimental

[Benjamin Saller]

PhantomJS Testing support

Change test server to use phantomjs. To get this branch working you'll need

sudo npm install -g phantomjs
sudo npm install -g mocha-phantomjs

After that make test-debug and test-prod should work with CLI exit codes
and .lbox.check should run the tests as well.

[Benjamin Saller (bcsaller)]

Reviewers: mp+144629_code.launchpad.net,

Message:
Please take a look.

Description:
PhantomJS Testing support

Change test server to use phantomjs. To get this branch working you'll
need

sudo npm install -g mocha-phantomjs

After that make test-debug and test-prod should work with CLI exit codes
and .lbox.check should run the tests as well.

https://code.launchpad.net/~bcsaller/juju-gui/phantom-testing/+merge/144629

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7201049/

Affected files:
   M .lbox.check
   M Makefile
   A [revision details]
   M app/app.js
   M app/assets/javascripts/d3-components.js
   M app/models/models.js
   M app/store/env.js
   M app/views/charm.js
   M app/views/environment.js
   M app/views/unit.js
   M app/views/utils.js
   M test-server.sh
   M test/index.html
   M test/test_env.js
   M test/utils.js

[Gary Poster (gary)]

Trivial comments. Thank you for doing this! It looks good. I will
give it a try tomorrow morning.

https://codereview.appspot.com/7201049/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7201049/diff/1/Makefile#newcode336
Makefile:336:
trivial, but might as well revert this.

https://codereview.appspot.com/7201049/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7201049/diff/1/app/app.js#newcode199
app/app.js:199: // Concession to testing, they need muck
Trivial, but I suggest simply saying "PhantomJS handles the console
itself, if used."

https://codereview.appspot.com/7201049/diff/1/app/app.js#newcode648
app/app.js:648: //console.log('load service', evt);
Could simply delete IMO

https://codereview.appspot.com/7201049/diff/1/app/assets/javascripts/d3-components.js
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/7201049/diff/1/app/assets/javascripts/d3-components.js#newcode319
app/assets/javascripts/d3-components.js:319: //console.debug('D3 Handler
for', selector, trigger);
Just delete it IMO

https://codereview.appspot.com/7201049/diff/1/app/assets/javascripts/d3-components.js#newcode497
app/assets/javascripts/d3-components.js:497: //console.log('Component',
methodName, 'on', name);
Just delete it IMO

https://codereview.appspot.com/7201049/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/7201049/diff/1/app/models/models.js#newcode452
app/models/models.js:452: //console.log('Reset Application Database');
Just delete it IMO

https://codereview.appspot.com/7201049/diff/1/app/models/models.js#newcode460
app/models/models.js:460: //console.log('Delta');
Just delete it IMO

https://codereview.appspot.com/7201049/diff/1/app/store/env.js
File app/store/env.js (right):

https://codereview.appspot.com/7201049/diff/1/app/store/env.js#newcode32
app/store/env.js:32: //console.log('Env Init');
Just delete it IMO

https://codereview.appspot.com/7201049/diff/1/app/store/env.js#newcode56
app/store/env.js:56: //console.log('Env Connect');
Just delete it IMO, and so on for all other commented-out console.log
statements.

https://codereview.appspot.com/7201049/

[Francesco Banconi (frankban)]

Thanks for this branch Ben, it's cool to have a meaningful exit code
from our test suite.

+1 on removing all commented "console.log" lines.

Unfortunately, I get this error while trying to run tests:

$ make test-prod
./test-server.sh prod
prod server listening on 8084
make: *** [test-prod] Error 127

Info on mocha-phantomjs:

mocha-phantomjs@1.1.3 /usr/local/lib/node_modules/mocha-phantomjs
├── commander@1.0.5 (keypress@0.1.0)
└── mocha@1.8.1 (growl@1.7.0, commander@0.6.1, diff@1.0.2, debug@0.7.0,
mkdirp@0.3.3, ms@0.3.0, jade@0.26.3)

https://codereview.appspot.com/7201049/

[Gary Poster (gary)]

Hey. I duped Francesco's failure and dug into it a bit for exercise :-)
. It turns out that the problem for us is simply that mocha-phantomjs
can't find phantomjs. This is the pertinent code in
/usr/bin/mocha-phantomjs:

   85 var phantomjs;
   86 for (var i=0; i < module.paths.length; i++) {
   87 var bin = path.join(module.paths[i], '.bin/phantomjs');
   88 if (process.platform === 'win32') {
   89 bin += '.cmd';
   90 }
   91 if (exists(bin)) {
   92 phantomjs = spawn(bin, spawnArgs);
   93 break;
   94 }
   95 }
   96 if (phantomjs === undefined) { phantomjs = spawn('phantomjs',
spawnArgs); }

module.paths for me was this:

[ '/usr/lib/node_modules/mocha-phantomjs/bin/node_modules',
   '/usr/lib/node_modules/mocha-phantomjs/node_modules',
   '/usr/lib/node_modules',
   '/usr/node_modules',
   '/node_modules' ]

.bin/phantomjs does not exist in any of those locations, and then the
last check for a global phantomjs fails.

I found that if I simply installed a global phantomjs (`sudo npm install
-g phantomjs`) the tests ran great.

Unless someone has objections to all of us also installing phantomjs
globally, these are my additional suggestions beyond the previous
trivial comments.

  * In the "Developer Install" section of the HACKING document, record
the instructions to install both mocha-phantomjs and phantomjs globally.
  (Note that orange squad is coming on soon and will need that HACKING
document to actually work!)

  * Make a separate Makefile target for simply starting the test server.
For now, I'd like for us to still be able to run the tests occasionally
in browsers, and in the future, this is the kind of thing we will need
for CI test services.

Thanks

Gary

https://codereview.appspot.com/7201049/

[Gary Poster (gary)]

To be clear, my vote is "land with changes"

Thanks

https://codereview.appspot.com/7201049/

[Benjamin Saller (bcsaller)]

On 2013/01/24 13:04:16, gary.poster wrote:
> Hey. I duped Francesco's failure and dug into it a bit for exercise
:-) . It
> turns out that the problem for us is simply that mocha-phantomjs can't
find
> phantomjs. This is the pertinent code in /usr/bin/mocha-phantomjs:

> 85 var phantomjs;
> 86 for (var i=0; i < module.paths.length; i++) {
> 87 var bin = path.join(module.paths[i], '.bin/phantomjs');
> 88 if (process.platform === 'win32') {
> 89 bin += '.cmd';
> 90 }
> 91 if (exists(bin)) {
> 92 phantomjs = spawn(bin, spawnArgs);
> 93 break;
> 94 }
> 95 }
> 96 if (phantomjs === undefined) { phantomjs = spawn('phantomjs',
spawnArgs); }

> module.paths for me was this:

> [ '/usr/lib/node_modules/mocha-phantomjs/bin/node_modules',
> '/usr/lib/node_modules/mocha-phantomjs/node_modules',
> '/usr/lib/node_modules',
> '/usr/node_modules',
> '/node_modules' ]

> .bin/phantomjs does not exist in any of those locations, and then the
last check
> for a global phantomjs fails.

> I found that if I simply installed a global phantomjs (`sudo npm
install -g
> phantomjs`) the tests ran great.

Thanks.

> Unless someone has objections to all of us also installing phantomjs
globally,
> these are my additional suggestions beyond the previous trivial
comments.

> * In the "Developer Install" section of the HACKING document, record
the
> instructions to install both mocha-phantomjs and phantomjs globally.
(Note that
> orange squad is coming on soon and will need that HACKING document to
actually
> work!)

> * Make a separate Makefile target for simply starting the test
server. For
> now, I'd like for us to still be able to run the tests occasionally in
browsers,
> and in the future, this is the kind of thing we will need for CI test
services.

Agreed. I got enthusiastic once I started to get it working. I think we
can default to prod for the integrated test and debug for browser (as
you'll most likely be using the debugger at this point).

> Thanks

> Gary

https://codereview.appspot.com/7201049/

[Benjamin Saller (bcsaller)]

*** Submitted:

PhantomJS Testing support

Change test server to use phantomjs. To get this branch working you'll need

sudo npm install -g phantomjs
sudo npm install -g mocha-phantomjs

After that make test-debug and test-prod should work with CLI exit codes
and .lbox.check should run the tests as well.

=== modified file '.lbox.check'
--- .lbox.check	2012-09-21 14:36:02 +0000
+++ .lbox.check	2013-01-24 04:15:26 +0000
@@ -1,3 +1,3 @@
 #!/bin/sh
 
-make lint
+make lint test-prod

=== modified file 'Makefile'
--- Makefile	2013-01-22 08:33:50 +0000
+++ Makefile	2013-01-24 04:15:26 +0000
@@ -333,7 +333,7 @@
 
 test-prod: build-prod
 	./test-server.sh prod
-
+	
 test:
 	@echo "Deprecated. Please run either 'make test-prod' or 'make"
 	@echo "test-debug', to test the production or debug environments"

=== modified file 'app/app.js'
--- app/app.js	2013-01-19 14:57:23 +0000
+++ app/app.js	2013-01-24 04:15:26 +0000
@@ -194,10 +194,16 @@
      */
     initializer: function() {
       // If this flag is true, start the application with the console activated
-      if (this.get('consoleEnabled')) {
-        consoleManager.native();
-      } else {
-        consoleManager.noop();
+      var consoleEnabled = this.get('consoleEnabled');
+
+      // Concession to testing, they need muck
+      // with console, we can't as well.
+      if (window.mochaPhantomJS === undefined) {
+        if (consoleEnabled) {
+          consoleManager.native();
+        } else {
+          consoleManager.noop();
+        }
       }
       // Create a client side database to store state.
       this.db = new models.Database();
@@ -236,13 +242,6 @@
         env: this.env,
         notifications: this.db.notifications});
 
-      // Event subscriptions
-
-      this.on('*:navigateTo', function(e) {
-        console.log('navigateTo', e);
-        this.navigate(e.url);
-      }, this);
-
       // Notify user attempts to modify the environment without permission.
       this.env.on('permissionDenied', this.onEnvPermissionDenied, this);
 
@@ -271,22 +270,13 @@
       // TODO - Bound views will automatically update this on individual models
       this.db.on('update', this.on_database_changed, this);
 
-      this.on('navigate', function(e) {
-        console.log('app navigate', e);
-      });
-
       this.enableBehaviors();
 
       this.once('ready', function(e) {
         if (this.get('socket_url')) {
           // Connect to the environment.
-          console.log('App: Connecting to environment');
           this.env.connect();
         }
-
-        console.log(
-            'App: Re-rendering current view', this.getPath(), 'info');
-
         if (this.get('activeView')) {
           this.get('activeView').render();
         } else {
@@ -381,8 +371,6 @@
      * @method show_unit
      */
     show_unit: function(req) {
-      console.log(
-          'App: Route: Unit', req.params.id, req.path, req.pendingRoutes);
       // This replacement honors service names that have a hyphen in them.
       var unit_id = req.params.id.replace(/^(\S+)-(\d+)$/, '$1/$2');
       var unit = this.db.units.getById(unit_id);
@@ -657,7 +645,7 @@
      * @method load_service
      */
     loadService: function(evt) {
-      console.log('load service', evt);
+      //console.log('load service', evt);
       if (evt.err) {
         this.db.notifications.add(
             new models.Notification({

=== modified file 'app/assets/javascripts/d3-components.js'
--- app/assets/javascripts/d3-components.js	2013-01-08 15:13:28 +0000
+++ app/assets/javascripts/d3-components.js	2013-01-24 04:15:26 +0000
@@ -253,7 +253,6 @@
                 // (re)Register the event to bubble.
                 self.publish(name, {emitFacade: true});
               }
-              console.debug('yui event binding', module.name, eventPhase, name);
               subscriptions.push(
                   target[eventPhase](
                   name, callback, handler.context));
@@ -317,7 +316,7 @@
           adapter = function() {
             var selection = d3.select(this),
                 d = selection.data()[0];
-            console.debug('D3 Handler for', selector, trigger);
+            //console.debug('D3 Handler for', selector, trigger);
             return handler.callback.call(this, d, handler.context);
           };
           d3.selectAll(selector).on(trigger, adapter);
@@ -495,7 +494,7 @@
     all: function(methodName) {
       Y.each(this.modules, function(mod, name) {
         if (methodName in mod) {
-          console.log('Component', methodName, 'on', name);
+          //console.log('Component', methodName, 'on', name);
           mod[methodName]();
         }
       });

=== modified file 'app/models/models.js'
--- app/models/models.js	2013-01-19 06:46:17 +0000
+++ app/models/models.js	2013-01-24 04:15:26 +0000
@@ -449,20 +449,18 @@
       this.relations.reset();
       this.units.reset();
       this.notifications.reset();
-      console.log('Reset Application Database');
+      //console.log('Reset Application Database');
     },
 
     on_delta: function(delta_evt) {
       var changes = delta_evt.data.result;
-      console.groupCollapsed('Delta');
-      console.log('Delta', this, changes);
       var change_type, model_class = null,
           self = this;
 
+      //console.log('Delta');
       changes.forEach(
           Y.bind(function(change) {
             change_type = change[0];
-            console.log('change', this, change);
             this.getModelListByModelName(change_type).process_delta(
                 change[1], change[2]);
           }, this));
@@ -470,7 +468,6 @@
         self.units.update_service_unit_aggregates(service);
       });
       this.fire('update');
-      console.groupEnd();
     }
 
   });

=== modified file 'app/store/env.js'
--- app/store/env.js	2013-01-18 09:57:41 +0000
+++ app/store/env.js	2013-01-24 04:15:26 +0000
@@ -29,7 +29,7 @@
     // Prototype methods for your new class
 
     initializer: function() {
-      console.log('Env Init');
+      //console.log('Env Init');
       // Define custom events
       this.publish('msg', {
         emitFacade: true,
@@ -53,10 +53,10 @@
     },
 
     connect: function() {
-      console.log('Env Connect');
+      //console.log('Env Connect');
       // Allow an external websocket to be passed in.
       var conn = this.get('conn');
-      console.log('ext ws', conn);
+      //console.log('ext ws', conn);
       if (conn) {
         this.ws = conn;
       } else {
@@ -72,21 +72,21 @@
     },
 
     on_open: function(data) {
-      console.log('Env: Connected');
+      //console.log('Env: Connected');
     },
 
     on_close: function(data) {
-      console.log('Env: Disconnect');
+      //console.log('Env: Disconnect');
       this.set('connected', false);
     },
 
     on_message: function(evt) {
-      console.log('Env: Receive', evt.data);
+      //console.log('Env: Receive', evt.data);
       var msg = Y.JSON.parse(evt.data);
       // The "ready" attribute indicates that this is a server's initial
       // greeting.  It provides a few initial values that we care about.
       if (msg.ready) {
-        console.log('Env: Handshake Complete');
+        //console.log('Env: Handshake Complete');
         this.set('connected', true);
         this.set('providerType', msg.provider_type);
         this.set('defaultSeries', msg.default_series);
@@ -114,7 +114,7 @@
     },
 
     dispatch_result: function(data) {
-      console.log('Env: Dispatch Result', data);
+      //console.log('Env: Dispatch Result', data);
       this._dispatch_rpc_result(data);
       this._dispatch_event(data);
     },
@@ -124,7 +124,7 @@
         console.warn('Env: Unknown evt kind', evt);
         return;
       }
-      console.log('Env: Dispatch Evt', evt.op);
+      //console.log('Env: Dispatch Evt', evt.op);
       this.fire(evt.op, {data: evt});
     },
 
@@ -132,7 +132,7 @@
       if ('request_id' in msg) {
         var tid = msg.request_id;
         if (tid in this._txn_callbacks) {
-          console.log('Env: Dispatch Rpc');
+          //console.log('Env: Dispatch Rpc');
           this._txn_callbacks[tid].apply(this, [msg]);
           delete this._txn_callbacks[tid];
         }
@@ -174,7 +174,7 @@
       }
       op.request_id = tid;
       var msg = Y.JSON.stringify(op);
-      console.log('Env: send msg', tid, msg, op);
+      //console.log('Env: send msg', tid, msg, op);
       this.ws.send(msg);
     },
 
@@ -258,7 +258,7 @@
      */
     deploy: function(charm_url, service_name, config, config_raw, num_units,
                      callback) {
-      console.log(charm_url, service_name, config, config_raw, num_units);
+      //console.log(charm_url, service_name, config, config_raw, num_units);
       this._send_rpc(
           { op: 'deploy',
             service_name: service_name,

=== modified file 'app/views/charm.js'
--- app/views/charm.js	2012-11-08 16:23:16 +0000
+++ app/views/charm.js	2013-01-24 04:15:26 +0000
@@ -61,7 +61,7 @@
     on_charm_data: function(io_request) {
       var charm = Y.JSON.parse(
           io_request.response.results[0].responseText);
-      console.log('results update', charm, this);
+      //console.log('results update', charm, this);
       this.set('charm', charm);
       this.render();
     },
@@ -71,7 +71,7 @@
           container = this.get('container'),
           charmUrl = charm.series + '/' + charm.name,
           env = this.get('env');
-      console.log('charm deploy', charm);
+      //console.log('charm deploy', charm);
       // Generating charm url: see http://jujucharms.com/tools/store-missing
       // for examples of charm addresses.
       if (charm.owner !== 'charmers') {
@@ -166,7 +166,7 @@
     on_results_change: function(io_request) {
       var result_set = Y.JSON.parse(
           io_request.response.results[0].responseText);
-      console.log('results update', result_set, this);
+      //console.log('results update', result_set, this);
       this.set('charms', result_set.results);
       this.render();
     }

=== modified file 'app/views/environment.js'
--- app/views/environment.js	2013-01-21 15:08:07 +0000
+++ app/views/environment.js	2013-01-24 04:15:26 +0000
@@ -22,7 +22,6 @@
                                       [views.JujuBaseView],
       {
         initializer: function() {
-          console.log('View: Initialized: Env');
           this.publish('navigateTo', {
             broadcast: true,
             preventable: false});

=== modified file 'app/views/unit.js'
--- app/views/unit.js	2012-11-08 16:23:16 +0000
+++ app/views/unit.js	2013-01-24 04:15:26 +0000
@@ -21,7 +21,7 @@
    */
   var UnitView = Y.Base.create('UnitView', Y.View, [], {
     initializer: function() {
-      console.log('view.init.unit', this.get('unit'));
+      //console.log('view.init.unit', this.get('unit'));
     },
 
     template: Templates.unit,
@@ -34,7 +34,7 @@
      */
     render: function() {
       var container = this.get('container');
-      console.log('view.render.unit');
+      //console.log('view.render.unit');
       var unit = this.get('unit');
       if (!unit) {
         container.setHTML('<div class="alert">Loading...</div>');

=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-01-17 20:28:28 +0000
+++ app/views/utils.js	2013-01-24 04:15:26 +0000
@@ -553,8 +553,8 @@
   };
 
   utils.validate = function(values, schema) {
-    console.group('view.utils.validate');
-    console.log('validating', values, 'against', schema);
+    //console.group('view.utils.validate');
+    //console.log('validating', values, 'against', schema);
     var errors = {};
 
     function toString(value) {
@@ -574,7 +574,7 @@
 
     Y.Object.each(schema, function(field_definition, name) {
       var value = toString(values[name]);
-      console.log('validating field', name, 'with value', value);
+      //console.log('validating field', name, 'with value', value);
 
       if (field_definition.type === 'int') {
         if (!value) {
@@ -597,10 +597,10 @@
         }
       }
 
-      console.log('generated this error (possibly undefined)', errors[name]);
+      //console.log('generated this error (possibly undefined)', errors[name]);
     });
-    console.log('returning', errors);
-    console.groupEnd();
+    //console.log('returning', errors);
+    //console.groupEnd();
     return errors;
   };
 
@@ -996,7 +996,7 @@
   });
 
   Y.Handlebars.registerHelper('iflat', function(iface_decl, options) {
-    // console.log('helper', iface_decl, options, this);
+    //console.log('helper', iface_decl, options, this);
     var result = [];
     var ret = '';
     Y.Object.each(iface_decl, function(value, name) {

=== modified file 'test-server.sh'
--- test-server.sh	2012-12-07 17:57:27 +0000
+++ test-server.sh	2013-01-24 04:15:26 +0000
@@ -4,5 +4,7 @@
 
 node ./test-server.js $1 &
 sleep 2
-xdg-open http://localhost:8084/test/
-fg %1
+mocha-phantomjs http://localhost:8084/test/index.html
+status=$?
+kill %1
+exit $status

=== modified file 'test/index.html'
--- test/index.html	2013-01-17 16:19:17 +0000
+++ test/index.html	2013-01-24 04:15:26 +0000
@@ -10,12 +10,11 @@
   <script src="assets/mocha.js"></script>
   <script>
     var assert = chai.assert,
-        expect = chai.expect;
-
-    var should = chai.should();
-    console.log('mocha setup');
-    mocha.setup({'ui': 'bdd', 'ignoreLeaks': false, 'timeout': 20000})
-    console.log('mocha setup done');
+        expect = chai.expect,
+        should = chai.should();
+    mocha.reporter('html');
+    mocha.ui('bdd');
+    mocha.setup({ignoreLeaks: false, timeout: 20000})
   </script>
 
   <!-- Load up YUI base, app modules, and test utils -->
@@ -61,12 +60,15 @@
   YUI_config = {
       async: false,
       consoleEnabled: true,
-      delayUntil: 'domready'
+      delayUntil: 'domready',
+      debug: false,
+      filter: 'debug'
   };
 
   YUI().use(['node', 'event'], function(Y) {
-     // Run the tests.
-      if (window.mochaPhantomJS) { mochaPhantomJS.run(); }
+      // Run the tests.
+      if (window.mochaPhantomJS) {
+          mochaPhantomJS.run(); }
       else { mocha.run(); }
   });
   </script>

=== modified file 'test/test_env.js'
--- test/test_env.js	2013-01-18 09:57:41 +0000
+++ test/test_env.js	2013-01-24 04:15:26 +0000
@@ -59,14 +59,14 @@
 
     it('can accept a callback on its methods', function(done) {
       env.get_charm('cs:precise/mysql', function(result) {
-        console.log('invoked', result);
+        //console.log('invoked', result);
         result.op.should.equal('get_charm');
         result.result.id.should.equal('cs:precise/mysql');
         done();
       });
 
       msg = conn.last_message();
-      console.log('msg', msg);
+      //console.log('msg', msg);
       conn.msg({
         'op': 'get_charm',
         'request_id': msg.request_id,

=== modified file 'test/utils.js'
--- test/utils.js	2012-10-12 06:21:25 +0000
+++ test/utils.js	2013-01-24 04:15:26 +0000
@@ -24,7 +24,7 @@
       };
 
       this.msg = function(m) {
-        console.log('serializing env msg', m);
+        //console.log('serializing env msg', m);
         this.onmessage({'data': Y.JSON.stringify(m)});
       };
 
@@ -33,7 +33,7 @@
       };
 
       this.send = function(m) {
-        console.log('socket send', m);
+        //console.log('socket send', m);
         this.messages.push(Y.JSON.parse(m));
       };