Merge lp:~frankban/juju-gui/bug-1081803-tests-mutate-url into lp:juju-gui/experimental

[Francesco Banconi]

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/

[Francesco Banconi (frankban)]

Reviewers: mp+135670_code.launchpad.net,

Message:
Please take a look.

Description:
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://code.launchpad.net/~frankban/juju-gui/bug-1081803-tests-mutate-url/+merge/135670

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M test/test_app.js
   M test/test_app_hotkeys.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision:
<email address hidden>

Index: test/test_app.js
=== 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:20:52 +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', providerTy...

[Nicola Larosa (teknico)]

Nice cleanup, just a couple minor comments inline.

https://codereview.appspot.com/6856075/diff/1/test/test_app.js
File test/test_app.js (left):

https://codereview.appspot.com/6856075/diff/1/test/test_app.js#oldcode50
test/test_app.js:50: }
I'm a bit concerned about this: I wonder what exactly "losing the race"
means. It might warrant further investigation.

https://codereview.appspot.com/6856075/diff/1/test/test_app_hotkeys.js
File test/test_app_hotkeys.js (right):

https://codereview.appspot.com/6856075/diff/1/test/test_app_hotkeys.js#newcode41
test/test_app_hotkeys.js:41: keyCode: 83, // "S" key.
Is the additional space before the "S" useful?

https://codereview.appspot.com/6856075/diff/1/test/test_app_hotkeys.js#newcode58
test/test_app_hotkeys.js:58: keyCode: 69, // "E" key.
Same comment as line 41 above.

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

[lp:~frankban/juju-gui/bug-1081803-tests-mutate-url]

259. By Francesco Banconi on 2012-11-22

Clean up white spaces.

[Francesco Banconi (frankban)]

Thanks for the review Nicola.

On 2012/11/22 15:06:01, teknico wrote:
> Nice cleanup, just a couple minor comments inline.

https://codereview.appspot.com/6856075/diff/1/test/test_app.js#oldcode50
> test/test_app.js:50: }
> I'm a bit concerned about this: I wonder what exactly "losing the
race" means.
> It might warrant further investigation.

Maybe it means a subsequent test tries to create a container before it
is destroyed by the first one, but I'm just guessing. Anyway, as
mentioned, I was not able to reproduce that behavior, and we are doing
the same elsewhere. Also notice that removing the node in beforeEach
means it is not removed at all after the last test of that test case.

> https://codereview.appspot.com/6856075/diff/1/test/test_app_hotkeys.js
> File test/test_app_hotkeys.js (right):

https://codereview.appspot.com/6856075/diff/1/test/test_app_hotkeys.js#newcode41
> test/test_app_hotkeys.js:41: keyCode: 83, // "S" key.
> Is the additional space before the "S" useful?

Absolutely not. And it's also wrong. Removing them.

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

[Francesco Banconi (frankban)]

Please take a look.

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

[Gary Poster (gary)]

Really nice cleanups, Francesco. Thank you. Please land it!

Gary

https://codereview.appspot.com/6856075/diff/5001/test/test_app.js
File test/test_app.js (left):

https://codereview.appspot.com/6856075/diff/5001/test/test_app.js#oldcode44
test/test_app.js:44: // XXX Apparently removing a DOM node is
asynchronous (on Chrome at least)
Weird that this comment is not in trunk. Not worrying about it.

I agree that the tests seem reliable on Chrome without this code, so
ripping it out seems fine.

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

[lp:~frankban/juju-gui/bug-1081803-tests-mutate-url]

260. By Francesco Banconi on 2012-11-27

Merged trunk and resolved conflicts.

[Francesco Banconi (frankban)]

*** Submitted:

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.

R=
CC=
https://codereview.appspot.com/6856075

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

=== modified file 'test/test_app.js'
--- test/test_app.js	2012-11-23 16:21:32 +0000
+++ test/test_app.js	2012-11-27 14:27:31 +0000
@@ -36,9 +36,6 @@
     var app, container;
 
     beforeEach(function() {
-      if (container) {
-        container.remove(true);
-      }
       container = Y.one('#main')
         .appendChild(Y.Node.create('<div/>'))
           .set('id', 'test-container')
@@ -46,13 +43,18 @@
           .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});
       injectData(app);
     });
 
+    afterEach(function() {
+      container.remove(true);
+    });
+
     it('should produce a valid index', function() {
       var container = app.get('container');
       app.render();
@@ -104,14 +106,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-23 16:21:32 +0000
+++ test/test_app_hotkeys.js	2012-11-27 14:27:31 +0000
@@ -3,9 +3,14 @@
 YUI(GlobalConfig).use(['juju-gui', 'juju-tests-utils', 'node-event-simulate'],
     function(Y) {
       describe('application hotkeys', function() {
-        var app, container, env, conn, testUtils, windowNode;
+        var app, container, windowNode;
 
-        before(function() {
+        before(function(done) {
+          var env = {
+            after: function() {},
+            get: function() {},
+            on: function() {}
+          };
           windowNode = Y.one(window);
           app = new Y.juju.App({
             env: env,
@@ -13,51 +18,46 @@
             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);
         });
+
       });
     });