yellow team mailing list archive

[Kapil Thangavelu <kapil.thangavelu@xxxxxxxxxxxxx>]

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.

https://codereview.appspot.com/7003054/

-- 
https://code.launchpad.net/~hazmat/juju-gui/reliable-test/+merge/141197
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~hazmat/juju-gui/reliable-test into lp:juju-gui.