yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Francesco Banconi has proposed merging lp:~frankban/juju-gui/bug-1081803-tests-mutate-url into lp:juju-gui.

Requested reviews:
  Juju GUI Hackers (juju-gui)
Related bugs:
  Bug #1081803 in juju-gui: "tests mutate/break uri"
  https://bugs.launchpad.net/juju-gui/+bug/1081803

For more details, see:
https://code.launchpad.net/~frankban/juju-gui/bug-1081803-tests-mutate-url/+merge/135670

Tests mutating URL and not removing test nodes.

One of the hotkeys tests mutated the URL.
Both tests in that test case also failed when run
in isolation, and some variables were unused.

Also fixed tests leaving elements in the DOM:
as a consequence, "Environmenton excellent provider"
was visible at the top of the page after a test run.
Now the container used by those tests is removed
after each test is run. Also removed an XXX saying
that removing the elements in afterEach can be
problematic. I've tried to reproduce the erroneus
behavior described, running the test suite several
times, without success. Anyway, removing in afterEach
what was created in beforeEach is something we do
everywhere in the tests. So, that XXX confused me 
a bit, any hint?

To avoid flickering in the test page, I tried to
hide the nodes used by tests where possible.

https://codereview.appspot.com/6856075/

-- 
https://code.launchpad.net/~frankban/juju-gui/bug-1081803-tests-mutate-url/+merge/135670
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/juju-gui/bug-1081803-tests-mutate-url into lp:juju-gui.

=== modified file 'test/test_app.js'
--- test/test_app.js	2012-11-14 14:34:31 +0000
+++ test/test_app.js	2012-11-22 12:54:26 +0000
@@ -41,13 +41,6 @@
   });
 
   beforeEach(function(done) {
-    //  XXX Apparently removing a DOM node is asynchronous (on Chrome at least)
-    //  and we occasionally lose the race if this code is in the afterEach
-    //  function, so instead we do it here, but only if one has previously been
-    //  created.
-    if (container) {
-      container.remove(true);
-    }
     container = Y.one('#main')
       .appendChild(Y.Node.create('<div/>'))
         .set('id', 'test-container')
@@ -55,7 +48,8 @@
         .append(Y.Node.create('<span/>')
           .set('id', 'environment-name'))
         .append(Y.Node.create('<span/>')
-          .set('id', 'provider-type'));
+          .set('id', 'provider-type'))
+        .hide();
     app = new Y.juju.App(
         { container: container,
           viewContainer: container});
@@ -63,6 +57,10 @@
     done();
   });
 
+  afterEach(function() {
+    container.remove(true);
+  });
+
   it('should produce a valid index', function() {
     var container = app.get('container');
     app.render();
@@ -114,14 +112,13 @@
   it('should show the provider type, when available', function() {
     var providerType = 'excellent provider';
     // Since no provider type has been set yet, none is displayed.
-    assert.equal(
-        container.one('#provider-type').get('text'),
-        '');
+    assert.equal('', container.one('#provider-type').get('text'));
     app.env.set('providerType', providerType);
     // The provider type has been displayed.
     assert.equal(
-        container.one('#provider-type').get('text'),
-        'on ' + providerType);
+        'on ' + providerType,
+        container.one('#provider-type').get('text')
+    );
   });
 
 });

=== modified file 'test/test_app_hotkeys.js'
--- test/test_app_hotkeys.js	2012-11-20 15:39:46 +0000
+++ test/test_app_hotkeys.js	2012-11-22 12:54:26 +0000
@@ -1,12 +1,17 @@
 'use strict';
 
 describe('application hotkeys', function() {
-  var Y, app, container, env, conn, testUtils, windowNode;
+  var Y, app, container, windowNode;
 
-  before(function() {
+  before(function(done) {
     Y = YUI(GlobalConfig).use(
         ['juju-gui', 'juju-tests-utils',
           'node-event-simulate'], function(Y) {
+          var env = {
+            after: function() {},
+            get: function() {},
+            on: function() {}
+          };
           windowNode = Y.one(window);
           app = new Y.juju.App({
             env: env,
@@ -14,48 +19,43 @@
             viewContainer: container
           });
           app.activateHotkeys();
+          done();
         });
   });
 
+  beforeEach(function() {
+    container = Y.Node.create('<div/>');
+    Y.one('#main').append(container);
+    app.render();
+  });
+
   afterEach(function() {
     container.remove(true);
   });
 
-  beforeEach(function() {
-    container = Y.one('#main').appendChild(Y.Node.create('<div/>')).set('id',
-        'test-container').append(
-        Y.Node.create('<input />').set('id', 'charm-search-field'));
-    testUtils = Y.namespace('juju-tests.utils');
-    env = {
-      get: function() {
-      },
-      on: function() {
-      },
-      after: function() {
-      }
-    };
-  });
-
   it('should listen for alt-S events', function() {
-    app.render();
+    var searchInput = Y.Node.create('<input/>');
+    searchInput.set('id', 'charm-search-field');
+    container.append(searchInput);
     windowNode.simulate('keydown', {
-      keyCode: 83, // "S" key
+      keyCode: 83, //  "S" key.
       altKey: true
     });
     // Did charm-search-field get the focus?
-    assert.equal(Y.one('#charm-search-field'), Y.one(document.activeElement));
+    assert.equal(searchInput, Y.one(document.activeElement));
   });
 
   it('should listen for alt-E events', function() {
     var altEtriggered = false;
-    app.on('navigateTo', function(param) {
-      if (param && param.url === '/') {
+    app.on('navigateTo', function(ev) {
+      if (ev && ev.url === '/') {
         altEtriggered = true;
       }
+      // Avoid URL change performed by additional listeners.
+      ev.stopImmediatePropagation();
     });
-    app.render();
     windowNode.simulate('keydown', {
-      keyCode: 69, // "E" key
+      keyCode: 69, //  "E" key.
       altKey: true
     });
     assert.isTrue(altEtriggered);