Make tests more reliable.

Make tests more reliable.

- Disable async loading for yui to ensure app code is loaded before tests.
- Yank yui loader closures around tests, this is an anti-pattern.
- Any test file can be run in isolation now.
- Re-order test loading into alphabetical order to detect/correct collisions.
- Enable using firefox for tests via test loader fix.
- WIP enable using phantomjs for running tests on cli.
- Make topology-mega.js more foregiving on its (there's a event subscription leak here.)
- Fix event subscription leak in charm-panel code.

https://code.launchpad.net/~hazmat/juju-gui/reliable-test/+merge/141197

(do not edit description out of merge proposal)

[hazmat, 2012-12-24 03:49:24]

Please take a look.

[bcsaller, 2012-12-24 18:26:11]

This looks good, and looks like a real improvement, I made some changes below
that I think help as well. 

Not sure why Y.use around the tests is an anti-pattern. With this style of
loading it all up front we could almost get away with use('*') which would bind
all the modules to the Y object but that gives up something about scoped
dependencies in the tests. Happy with the improvement.

https://codereview.appspot.com/7003054/diff/1/test/index.html
File test/index.html (left):

https://codereview.appspot.com/7003054/diff/1/test/index.html#oldcode44
test/index.html:44: <script>
I changed to a pre-load global config object and added use of the delayUntil
option. I think this makes this already nice improvement a little nicer.

=== modified file 'test/index.html'
--- test/index.html	2012-12-24 03:17:11 +0000
+++ test/index.html	2012-12-24 18:21:50 +0000
@@ -53,29 +53,29 @@
 
 
   <script>
-  YUI({'async': false}).use('node', 'event', function(Y) {
-     Y.on('domready', function() {
-
-     var config = GlobalConfig;
-
-     config.async = false;
-     config.consoleEnabled = true;
-
-     for (group in config.groups) {
+  YUI_config = {
+      async: false,
+      consoleEnabled: true,
+      delayUntil: 'domready'
+  };
+
+  YUI().use(['node', 'event'], function(Y) {
+      var config = GlobalConfig;
+
+      for (group in config.groups) {
           var group = config.groups[group];
-         for (m in group.modules) {
-            var resource = group.modules[m];
-            if (!m || !resource.fullpath) {
-              continue
-            }
-            resource.fullpath = resource.fullpath.replace(
-              '/juju-ui/', '../juju-ui/');
-         }
-     }
-     // Run the tests.
-     if (window.mochaPhantomJS) { mochaPhantomJS.run(); }
-     else { mocha.run(); }
-     });
+          for (m in group.modules) {
+              var resource = group.modules[m];
+              if (!m || !resource.fullpath) {
+                  continue
+              }
+              resource.fullpath = resource.fullpath.replace(
+                  '/juju-ui/', '../juju-ui/');
+          }
+      }
+      // Run the tests.
+      if (window.mochaPhantomJS) { mochaPhantomJS.run(); }
+      else { mocha.run(); }
   });
   </script>

[bac, 2013-01-03 13:43:00]

These changes look good and should help standardize the way the tests are run. 
Like Ben I'm curious as to the new pattern for Y.use in the tests.

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

https://codereview.appspot.com/7003054/diff/1/app/app.js#newcode754
app/app.js:754: // This alias doesn't seem to work, including refs by hand.
It might be clear to say

The juju-controllers alias...

And if it doesn't work can we get rid of it?

https://codereview.appspot.com/7003054/diff/1/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/7003054/diff/1/app/modules-debug.js#newcode19
app/modules-debug.js:19: 
Why are we combining for the development use?  Won't that make debugging more
difficult?

https://codereview.appspot.com/7003054/diff/1/app/views/topology/panzoom.js
File app/views/topology/panzoom.js (right):

https://codereview.appspot.com/7003054/diff/1/app/views/topology/panzoom.js#n...
app/views/topology/panzoom.js:187: 'd3',
Just curious why you did the re-ordering.  Is it required?  Or are you just
grouping by hierarchy?

https://codereview.appspot.com/7003054/diff/1/test/index.html
File test/index.html (right):

https://codereview.appspot.com/7003054/diff/1/test/index.html#newcode31
test/index.html:31: <!-- Tests (Alphabetical)-->
Thanks!

https://codereview.appspot.com/7003054/diff/1/test/index.html#newcode61
test/index.html:61: config.async = false;
Is setting async in the YUI() call and here in the config necessary?  A comment
explaining why the redundancy is required might be helpful.

[hazmat, 2013-01-03 13:46:14]

On 2012/12/24 18:26:11, bcsaller wrote:
> This looks good, and looks like a real improvement, I made some changes below
> that I think help as well. 
> 
> Not sure why Y.use around the tests is an anti-pattern. With this style of
> loading it all up front we could almost get away with use('*') which would
bind
> all the modules to the Y object but that gives up something about scoped
> dependencies in the tests. Happy with the improvement.

in practice Y.use around the tests made test loading unreliable. mocha scan
would often complete before the Y.use had finished resulting in no tests founds
for a module.

> 
> https://codereview.appspot.com/7003054/diff/1/test/index.html
> File test/index.html (left):
> 
> https://codereview.appspot.com/7003054/diff/1/test/index.html#oldcode44
> test/index.html:44: <script>
> I changed to a pre-load global config object and added use of the delayUntil
> option. I think this makes this already nice improvement a little nicer.
> 
> === modified file 'test/index.html'
> --- test/index.html	2012-12-24 03:17:11 +0000
> +++ test/index.html	2012-12-24 18:21:50 +0000
> @@ -53,29 +53,29 @@
>  
>  
>    <script>
> -  YUI({'async': false}).use('node', 'event', function(Y) {
> -     Y.on('domready', function() {
> -
> -     var config = GlobalConfig;
> -
> -     config.async = false;
> -     config.consoleEnabled = true;
> -
> -     for (group in config.groups) {
> +  YUI_config = {
> +      async: false,
> +      consoleEnabled: true,
> +      delayUntil: 'domready'
> +  };
> +
> +  YUI().use(['node', 'event'], function(Y) {
> +      var config = GlobalConfig;
> +
> +      for (group in config.groups) {
>            var group = config.groups[group];
> -         for (m in group.modules) {
> -            var resource = group.modules[m];
> -            if (!m || !resource.fullpath) {
> -              continue
> -            }
> -            resource.fullpath = resource.fullpath.replace(
> -              '/juju-ui/', '../juju-ui/');
> -         }
> -     }
> -     // Run the tests.
> -     if (window.mochaPhantomJS) { mochaPhantomJS.run(); }
> -     else { mocha.run(); }
> -     });
> +          for (m in group.modules) {
> +              var resource = group.modules[m];
> +              if (!m || !resource.fullpath) {
> +                  continue
> +              }
> +              resource.fullpath = resource.fullpath.replace(
> +                  '/juju-ui/', '../juju-ui/');
> +          }
> +      }
> +      // Run the tests.
> +      if (window.mochaPhantomJS) { mochaPhantomJS.run(); }
> +      else { mocha.run(); }
>    });
>    </script>


looks nice, thanks. i'll give it a shot.