|
|
|
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)
Patch Set 1 #
Total comments: 6
Total messages: 4
|
hazmat
Please take a look.
|
13 years ago (2012年12月24日 03:49:24 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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>
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.
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.