Merge lp:~bac/juju-gui/1083935 into lp:juju-gui/experimental

[Brad Crittenden]

Add tests for panzoom.

Tests are added for panzoom. Due to agreed upon time-boxing, the testing is
not thorough but is a start. Another bug to complete the testing will be
filed.

https://codereview.appspot.com/7001047/

[Brad Crittenden (bac)]

Reviewers: mp+141135_code.launchpad.net,

Message:
Please take a look.

Description:
Add tests for panzoom.

Tests are added for panzoom. Due to agreed upon time-boxing, the
testing is
not thorough but is a start. Another but to complete the testing will
be
filed.

https://code.launchpad.net/~bac/juju-gui/1083935/+merge/141135

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/views/topology/mega.js
   M app/views/topology/panzoom.js
   M test/index.html
   M test/test_application_notifications.js
   A test/test_panzoom.js

[Madison Scott-Clary (makyo)]

Looks good, few minors, land otherwise.

In the past, we've been taking mocha's style pretty seriously and
treating the it function as the first word in a sentence. I know that
we may be changing testing frameworks soon, so it's up to you, but it
might be more readable to continue that with this package of tests
(i.e.: it('should set initial values').) It's a minor naming thing, and
I'm not too hung up on it, so if everyone else is cool with it, so am I
:)

https://codereview.appspot.com/7001047/

[Gary Poster (gary)]

Land with changes.

Hey Brad. Thank you. Looks good.

Actually, unrelated to your work, but new branches are broken right now
because of the new d3. Could you change packages.json to specify d3 in
this way?

"d3": "2.10.x"

I agree with Matt that it would be nice if you tried to make a sentence
with "it." I also agree that I'm kinda low on caring.

I wish you didn't have to redefine Y, but I'm guessing you did it for a
reason. Please at least try to change the async: false approach that
Kapil suggests for the "before" Y, and then delete the "beforeEach"
version, and see if that works. If not, land it.

Thanks

Gary

https://codereview.appspot.com/7001047/diff/1/test/test_panzoom.js
File test/test_panzoom.js (right):

https://codereview.appspot.com/7001047/diff/1/test/test_panzoom.js#newcode23
test/test_panzoom.js:23: Y = YUI(GlobalConfig).use(['node',
Why can't you just use the Y from before? This is a pattern I have not
seen before in our tests.

https://codereview.appspot.com/7001047/

[Brad Crittenden (bac)]

I'd just read this post by jml and thought I'd put it in practice,
neglecting the inherited style:

https://plus.google.com/115348217455779620753/posts/YA3ThKWhSAj

I've made the changes you request but perhaps we can bear this in mind
if we do change testing infrastructures so our descriptions can be more
precise.

https://codereview.appspot.com/7001047/

[Brad Crittenden (bac)]

The changes suggested by Matt and Gary have been incorporated, except
for the 'async': false suggestion. I found removing the async in
beforeEach() was unnecessary but it was required in before() regardless
of setting async:false. Perhaps I have missed something. Will clarify
before landing.

https://codereview.appspot.com/7001047/diff/1/test/test_panzoom.js
File test/test_panzoom.js (right):

https://codereview.appspot.com/7001047/diff/1/test/test_panzoom.js#newcode23
test/test_panzoom.js:23: Y = YUI(GlobalConfig).use(['node',
This was just plain dumb and has been removed.

https://codereview.appspot.com/7001047/

[lp:~bac/juju-gui/1083935]

302. By Brad Crittenden on 2013-01-02

Fix mocha test names to conform, remove unneeded complexity in test setup.

[Brad Crittenden (bac)]

Please take a look.

https://codereview.appspot.com/7001047/

[Gary Poster (gary)]

Hey Brad. I'm out today as planned vacation, and I'm also unfortunately sick with fever. If you need another look from someone please ask around

Gary

On Jan 2, 2013, at 8:02 AM, <email address hidden> wrote:

> Please take a look.
>
> https://codereview.appspot.com/7001047/

[Brad Crittenden (bac)]

*** Submitted:

Add tests for panzoom.

Tests are added for panzoom. Due to agreed upon time-boxing, the
testing is
not thorough but is a start. Another bug to complete the testing will
be
filed.

R=matthew.scott, gary.poster
CC=
https://codereview.appspot.com/7001047

https://codereview.appspot.com/7001047/

=== modified file 'app/views/topology/mega.js'
--- app/views/topology/mega.js	2012-12-20 13:34:43 +0000
+++ app/views/topology/mega.js	2013-01-02 13:00:31 +0000
@@ -1289,12 +1289,12 @@
     },
 
     /*
-         * Actions to be called on clicking a service.
-         */
+     * Actions to be called on clicking a service.
+     */
     service_click_actions: {
       /*
-           * Default action: show or hide control panel.
-           */
+       * Default action: show or hide control panel.
+       */
       toggleControlPanel: function(m, view, context) {
         var container = view.get('container'),
                 cp = container.one('#service-menu');
 
=== modified file 'app/views/topology/panzoom.js'
--- app/views/topology/panzoom.js	2012-12-20 16:23:32 +0000
+++ app/views/topology/panzoom.js	2013-01-02 13:00:31 +0000
@@ -48,7 +48,6 @@
     renderSlider: function() {
       var self = this,
           topo = this.get('component'),
-          contianer = topo.get('container'),
           value = 100,
           currentScale = topo.get('scale');
 
 
=== modified file 'package.json'
--- package.json	2012-12-11 04:11:39 +0000
+++ package.json	2013-01-02 13:00:31 +0000
@@ -15,7 +15,7 @@
     "cryptojs": ">= 2.5.3"
   },
   "devDependencies": {
-    "d3": ">=2.10.1",
+    "d3": "2.10.x",
     "yui": ">=3.7.0",
     "mocha": "1.5.x",
     "express": "3.x",
 
=== modified file 'test/index.html'
--- test/index.html	2012-12-17 14:53:46 +0000
+++ test/index.html	2013-01-02 13:00:31 +0000
@@ -21,6 +21,7 @@
 
   <script src="test_d3_components.js"></script>
   <script src="test_topology.js"></script>
+  <script src="test_panzoom.js"></script>
   <script src="test_env.js"></script>
   <script src="test_model.js"></script>
   <script src="test_notifications.js"></script>
 
=== modified file 'test/test_application_notifications.js'
--- test/test_application_notifications.js	2012-12-13 11:44:57 +0000
+++ test/test_application_notifications.js	2013-01-02 13:00:31 +0000
@@ -4,7 +4,7 @@
   var _setTimeout, _viewsHighlightRow, db, ERR_EV, juju, models, NO_OP,
       viewContainer, views, Y;
 
-  before(function() {
+  before(function(done) {
     Y = YUI(GlobalConfig).use(['node',
       'juju-models',
       'juju-views',
@@ -16,6 +16,7 @@
       juju = Y.namespace('juju');
       models = Y.namespace('juju.models');
       views = Y.namespace('juju.views');
+      done();
     });
 
     ERR_EV = {
 
=== added file 'test/test_panzoom.js'
--- test/test_panzoom.js	1970-01-01 00:00:00 +0000
+++ test/test_panzoom.js	2013-01-02 13:00:31 +0000
@@ -0,0 +1,138 @@
+'use strict';
+
+describe('pan zoom module', function() {
+  var db, juju, models, viewContainer, views, Y, pz, topo, vis;
+
+  before(function(done) {
+    Y = YUI(GlobalConfig).use(['node',
+      'juju-models',
+      'juju-views',
+      'juju-gui',
+      'juju-env',
+      'juju-tests-utils',
+      'node-event-simulate'],
+    function(Y) {
+      juju = Y.namespace('juju');
+      models = Y.namespace('juju.models');
+      views = Y.namespace('juju.views');
+      done();
+    });
+  });
+
+  beforeEach(function() {
+    viewContainer = Y.Node.create('<div />');
+    viewContainer.appendTo(Y.one('body'));
+    viewContainer.hide();
+    db = new models.Database();
+    var view = new views.environment({container: viewContainer, db: db});
+    view.render();
+    view.postRender();
+    pz = view.topo.modules.PanZoomModule;
+    topo = pz.get('component');
+    vis = topo.vis;
+  });
+
+  afterEach(function() {
+    viewContainer.remove(true);
+  });
+
+  it('should set initial values',
+      function() {
+        pz._translate.should.eql([0, 0]);
+        pz._scale.should.equal(1.0);
+      });
+
+  // Test the zoom handler calculations.
+  it('should handle fractional values properly in zoom scale',
+     function() {
+       // Floor is used so the scale will round down.
+       var evt = { scale: 0.609 };
+       var rescaleCalled = false;
+       pz.rescale = function() {
+         rescaleCalled = true;
+       };
+       pz.zoomHandler(evt);
+       pz.slider.get('value').should.equal(60);
+       assert.isTrue(rescaleCalled);
+     });
+
+  it('should have an upper limit on the slider',
+     function() {
+       var evt = { scale: 3.5 };
+       var rescaleCalled = false;
+       pz.rescale = function() {
+         rescaleCalled = true;
+       };
+       pz.zoomHandler(evt);
+       pz.slider.get('value').should.equal(200);
+       assert.isTrue(rescaleCalled);
+     });
+
+  it('should have a lower limint on the slider',
+     function() {
+       var evt = { scale: 0.18 };
+       var rescaleCalled = false;
+       pz.rescale = function() {
+         rescaleCalled = true;
+       };
+       pz.zoomHandler(evt);
+       pz.slider.get('value').should.equal(25);
+       assert.isTrue(rescaleCalled);
+     });
+
+  // Test the zoom calculations.
+  it('should handle fractional values within the limit for rescale',
+     function(done) {
+       // Floor is used so the scale will round down.
+       var evt =
+           { scale: 0.609,
+             translate: 't'};
+       var rescaled = false;
+       topo.once('rescaled', function() {
+         rescaled = true;
+         done();
+       });
+       pz.rescale(vis, evt);
+       pz._scale.should.equal(0.609);
+       var expected = 'translate(' + evt.translate + ') scale(0.609)';
+       vis.attr('transform').should.equal(expected);
+       assert.isTrue(rescaled);
+     });
+
+  it('should set an upper limit for rescale',
+     function(done) {
+       var evt =
+           { scale: 2.1,
+             translate: 'u'};
+       var rescaled = false;
+       topo.once('rescaled', function() {
+         rescaled = true;
+         done();
+       });
+       topo.set('scale', 2.0);
+       pz.rescale(vis, evt);
+       pz._scale.should.equal(2.0);
+       var expected = 'translate(' + evt.translate + ') scale(2)';
+       vis.attr('transform').should.equal(expected);
+       assert.isTrue(rescaled);
+     });
+
+  it('should set a lower limit for rescale',
+     function(done) {
+       var evt =
+           { scale: 0.2,
+             translate: 'v'};
+       var rescaled = false;
+       topo.once('rescaled', function() {
+         rescaled = true;
+         done();
+       });
+       topo.set('scale', 0.25);
+       pz.rescale(vis, evt);
+       pz._scale.should.equal(0.25);
+       var expected = 'translate(' + evt.translate + ') scale(0.25)';
+       vis.attr('transform').should.equal(expected);
+       assert.isTrue(rescaled);
+     });
+
+});