Merge lp:~allenap/launchpad/localpackagediffs-filter-by-package-set-bug-809786-refactor into lp:launchpad

[Gavin Panella]

[r=benji][bug=809786] Refactor some DistroSeries JavaScript and add a declarative approach for running YUI tests.

This moves a few DistroSeries related bits of JavaScript around and
fixes some lint. The only thing that really needs reviewing is the new
code in lib/lp/app/javascript/testing/testrunner.js. Its doc comment:

/**
 * Merely loading this script into a page will cause it to look for a
 * list of suites in the document using the selector ul#suites>li. If
 * found, the text within each node is considered to be a test module
 * name. This is then loaded, and its "suite" property passed to
 * Runner.run().
 *
 * Here's how to declare the suites to run:
 *
 * <ul id="suites">
 * <li>lp.registry.distroseries.initseries.test</li>
 * </ul>
 *
 */

The test is then declared as a regular YUI module instead of
containing test run code.

[Benji York (benji)]

This branch looks good.

review: Approve (code)

=== modified file 'Makefile'
--- Makefile	2011-07-15 15:55:34 +0000
+++ Makefile	2011-07-18 11:54:37 +0000
@@ -25,11 +25,15 @@
 JS_BUILD := min
 endif
 
-JS_SOURCE_PATHS = -path './lib/lp/*/javascript/*' ! -path '*/tests/*' \
-    ! -path '*/testing/*' ! -path './lib/lp/services/*'
+define JS_LP_PATHS
+lib -path 'lib/lp/*/javascript/*' \
+! -path '*/tests/*' ! -path '*/testing/*' \
+! -path 'lib/lp/services/*'
+endef
+
 JS_YUI := $(shell utilities/yui-deps.py $(JS_BUILD:raw=))
 JS_OTHER := $(wildcard lib/canonical/launchpad/javascript/*/*.js)
-JS_LP := $(shell find $(JS_SOURCE_PATHS) -name '*.js' ! -name '.*.js' )
+JS_LP := $(shell find $(JS_LP_PATHS) -name '*.js')
 JS_ALL := $(JS_YUI) $(JS_OTHER) $(JS_LP)
 JS_OUT := $(LP_BUILT_JS_ROOT)/launchpad.js
 
@@ -164,7 +168,7 @@
 
 jsbuild_widget_css: bin/jsbuild
 	${SHHH} bin/jsbuild \
- 	    --srcdir lib/lp/app/javascript \
+	    --srcdir lib/lp/app/javascript \
 	    --builddir $(LP_BUILT_JS_ROOT)
 
 $(JS_LP): jsbuild_widget_css
@@ -425,7 +429,9 @@
 copy-apache-config:
 	# We insert the absolute path to the branch-rewrite script
 	# into the Apache config as we copy the file into position.
-	sed -e 's,%BRANCH_REWRITE%,$(shell pwd)/scripts/branch-rewrite.py,' configs/development/local-launchpad-apache > /etc/apache2/sites-available/local-launchpad
+	sed -e 's,%BRANCH_REWRITE%,$(shell pwd)/scripts/branch-rewrite.py,' \
+		configs/development/local-launchpad-apache > \
+		/etc/apache2/sites-available/local-launchpad
 	cp configs/development/local-vostok-apache \
 		/etc/apache2/sites-available/local-vostok
 	touch /var/tmp/bazaar.launchpad.dev/rewrite.log
 
=== modified file 'lib/lp/app/javascript/testing/testrunner.js'
--- lib/lp/app/javascript/testing/testrunner.js	2011-07-08 06:06:15 +0000
+++ lib/lp/app/javascript/testing/testrunner.js	2011-07-18 11:54:37 +0000
@@ -39,3 +39,32 @@
 };
 
 }, "0.1", {"requires": ["oop", "test", "console"]});
+
+
+/**
+ * Merely loading this script into a page will cause it to look for a
+ * list of suites in the document using the selector ul#suites>li. If
+ * found, the text within each node is considered to be a test module
+ * name. This is then loaded, and its "suite" property passed to
+ * Runner.run().
+ *
+ * Here's how to declare the suites to run:
+ *
+ *   <ul id="suites">
+ *     <li>lp.registry.distroseries.initseries.test</li>
+ *   </ul>
+ *
+ */
+YUI().use("event", function(Y) {
+    Y.on("domready", function() {
+        var suites = Y.all("ul#suites > li");
+        Y.each(suites, function(suite_node) {
+            var suite_name = suite_node.get("text");
+            Y.use("lp.testing.runner", suite_name, function(y) {
+                var module = y, parts = suite_name.split(".");
+                while (parts.length > 0) { module = module[parts.shift()]; }
+                y.lp.testing.Runner.run(module.suite);
+            });
+        });
+    });
+});
 
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-07-08 09:06:24 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-07-18 11:54:37 +0000
@@ -827,6 +827,8 @@
         super(DistroSeriesDifferenceBaseView, self).initialize()
 
     def initialize_sync_label(self, label):
+        # XXX: GavinPanella 2011-07-13 bug=809985: Good thing the app servers
+        # are running single threaded...
         self.__class__.actions.byname['actions.sync'].label = label
 
     @property
@@ -861,6 +863,8 @@
             SimpleTerm(diff, diff.id)
                     for diff in self.cached_differences.batch]
         diffs_vocabulary = SimpleVocabulary(terms)
+        # XXX: GavinPanella 2011-07-13 bug=809985: Good thing the app servers
+        # are running single threaded...
         choice = self.form_fields['selected_differences'].field.value_type
         choice.vocabulary = diffs_vocabulary
 
 
=== renamed file 'lib/lp/registry/javascript/distroseries.js' => 'lib/lp/registry/javascript/distroseries.initseries.js'
--- lib/lp/registry/javascript/distroseries.js	2011-06-27 17:04:27 +0000
+++ lib/lp/registry/javascript/distroseries.initseries.js	2011-07-18 11:54:37 +0000
@@ -22,7 +22,9 @@
  *
  * @class FormRowWidget
  */
-var FormRowWidget = function() {
+var FormRowWidget;
+
+FormRowWidget = function() {
     FormRowWidget.superclass.constructor.apply(this, arguments);
 };
 
@@ -166,7 +168,9 @@
  * can also be made an overlay, and a component and a pocket selected.
  *
  */
-var ParentSeriesListWidget = function() {
+var ParentSeriesListWidget;
+
+ParentSeriesListWidget = function() {
     ParentSeriesListWidget
         .superclass.constructor.apply(this, arguments);
 };
@@ -466,7 +470,9 @@
  *
  * @class ChoiceListWidget
  */
-var ChoiceListWidget = function() {
+var ChoiceListWidget;
+
+ChoiceListWidget = function() {
     ChoiceListWidget.superclass.constructor.apply(this, arguments);
 };
 
@@ -660,7 +666,9 @@
  *
  * @class ArchitecturesChoiceListWidget
  */
-var ArchitecturesChoiceListWidget = function() {
+var ArchitecturesChoiceListWidget;
+
+ArchitecturesChoiceListWidget = function() {
     ArchitecturesChoiceListWidget
         .superclass.constructor.apply(this, arguments);
 };
@@ -726,14 +734,15 @@
      */
     remove_distroseries: function(distroseries_id) {
         // Compute which das is only in the distroseries to be removed.
-        arch_to_remove = [];
+        var arch_to_remove = [];
         var das = this._distroseries[distroseries_id];
         var i, ds, j;
         for (i=0; i<das.entries.length; i++) {
-            remove_das = true;
-            arch = das.entries[i].get('architecture_tag');
+            var remove_das = true;
+            var arch = das.entries[i].get('architecture_tag');
             for (ds in this._distroseries) {
-                if (ds !== distroseries_id) {
+                if (this._distroseries.hasOwnProperty(ds) &&
+                    ds !== distroseries_id) {
                    var other_das = this._distroseries[ds];
                    for (j=0; j<other_das.entries.length; j++) {
                        var other_arch = other_das.entries[j].get(
@@ -782,7 +791,9 @@
  *
  * @class SelectWidget
  */
-var SelectWidget = function() {
+var SelectWidget;
+
+SelectWidget = function() {
     SelectWidget.superclass.constructor.apply(this, arguments);
 };
 
@@ -949,7 +960,9 @@
  *
  * @class PackagesetPickerWidget
  */
-var PackagesetPickerWidget = function() {
+var PackagesetPickerWidget;
+
+PackagesetPickerWidget = function() {
     PackagesetPickerWidget
         .superclass.constructor.apply(this, arguments);
 };
@@ -1144,7 +1157,9 @@
  *
  * @class FormActionsWidget
  */
-var FormActionsWidget = function() {
+var FormActionsWidget;
+
+FormActionsWidget = function() {
     FormActionsWidget
         .superclass.constructor.apply(this, arguments);
 };
@@ -1234,7 +1249,9 @@
  *
  * @class DeriveDistroSeriesActionsWidget
  */
-var DeriveDistroSeriesActionsWidget = function() {
+var DeriveDistroSeriesActionsWidget;
+
+DeriveDistroSeriesActionsWidget = function() {
     DeriveDistroSeriesActionsWidget
         .superclass.constructor.apply(this, arguments);
 };
@@ -1333,7 +1350,8 @@
         add_parent_series(result);
     };
 
-    parent_picker = Y.lp.app.picker.create('DistroSeriesDerivation', config);
+    var parent_picker =
+        Y.lp.app.picker.create('DistroSeriesDerivation', config);
     parent_picker.show();
 };
 
@@ -1354,17 +1372,21 @@
  *
  * @function setup
  */
-namespace.setup = function() {
-    var form_actions = namespace.setupWidgets();
-    namespace.setupInteraction(form_actions);
+namespace.setup = function(cache) {
+    var form_actions = namespace.setupWidgets(cache);
+    namespace.setupInteraction(form_actions, cache);
 };
 
 /**
  * Setup the widgets objects and return the form object.
  *
  * @function setupWidgets
+ * @param {Object} cache Specify the value cache to use. If not
+ *     specified, LP.cache is used. Intended chiefly for testing.
  */
-namespace.setupWidgets = function() {
+namespace.setupWidgets = function(cache) {
+    if (cache === undefined) { cache = LP.cache; }
+
     var form_container = Y.one("#initseries-form-container");
     var form_table = form_container.one("table.form");
     var form_table_body = form_table.append(Y.Node.create('<tbody />'));
@@ -1394,7 +1416,7 @@
                      "architectures)."))
             .render(form_table_body);
     var packageset_choice = null;
-    if (LP.cache['is_first_derivation']) {
+    if (cache.is_first_derivation) {
         packageset_choice =
             new PackagesetPickerWidget()
                 .set("name", "field.packagesets")
@@ -1424,7 +1446,7 @@
             .render(form_table_body);
     var form_actions =
         new DeriveDistroSeriesActionsWidget({
-            context: LP.cache.context,
+            context: cache.context,
             srcNode: form_container.one("div.actions"),
             deriveFromChoices: parents_selection,
             architectureChoice: architecture_choice,
@@ -1440,10 +1462,14 @@
  * Setup the interaction between the widgets.
  *
  * @function setupInteraction
- * @param {DeriveDistroSeriesActionsWidget} The form widget containing all
- *     the other widgets.
+ * @param {DeriveDistroSeriesActionsWidget} The form widget containing
+ *     all the other widgets.
+ * @param {Object} cache Specify the value cache to use. If not
+ *     specified, LP.cache is used. Intended chiefly for testing.
  */
-namespace.setupInteraction = function(form_actions) {
+namespace.setupInteraction = function(form_actions, cache) {
+    if (cache === undefined) { cache = LP.cache; }
+
     // Wire up the add parent series link.
     var link = Y.one('#add-parent-series');
     if (Y.Lang.isValue(link)) {
@@ -1458,7 +1484,7 @@
         }
     });
 
-    if (LP.cache['is_first_derivation']) {
+    if (cache.is_first_derivation) {
         // Wire the architecture and packageset pickers to the parent picker.
         Y.on('parent_added', function(parent) {
             form_actions.architectureChoice.add_distroseries(parent);
@@ -1471,7 +1497,7 @@
         });
     }
     else {
-        // Disable rebuilding if LP.cache['is_first_derivation'] is false.
+        // Disable rebuilding if cache.is_first_derivation is false.
         form_actions.packageCopyOptions.fieldNode
             .one('input[value="Copy Source and Rebuild"]')
             .set('disabled', 'disabled');
@@ -1485,13 +1511,12 @@
             "if you want to use all the available " +
             "architectures)."));
         form_actions.architectureChoice.add_distroseries(
-            LP.cache['previous_series']);
+            cache.previous_series);
         // Setup the pre-selected parents (parents from the previous series).
-        var index;
-        for (index in LP.cache['previous_parents']) {
-            form_actions.deriveFromChoices.add_parent(
-                LP.cache['previous_parents'][index]);
-        }
+        Y.each(
+            cache.previous_parents,
+            form_actions.deriveFromChoices.add_parent,
+            form_actions.deriveFromChoices);
     }
 
     // Wire up the form submission.
 
=== renamed file 'lib/lp/registry/javascript/tests/test_distroseries.html' => 'lib/lp/registry/javascript/tests/test_distroseries.initseries.html'
--- lib/lp/registry/javascript/tests/test_distroseries.html	2011-07-08 06:06:15 +0000
+++ lib/lp/registry/javascript/tests/test_distroseries.initseries.html	2011-07-18 11:54:37 +0000
@@ -6,27 +6,40 @@
   <title>Launchpad DistroSeries</title>
 
   <!-- YUI and test setup -->
-  <script type="text/javascript"
-          src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
-  </script>
   <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
   <script type="text/javascript"
+          src="../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+  <script type="text/javascript"
           src="../../../app/javascript/testing/testrunner.js"></script>
 
-    <!-- Required modules -->
-    <script type="text/javascript" src="../../../app/javascript/client.js"></script>
-    <script type="text/javascript" src="../../../app/javascript/activator/activator.js"></script>
-    <script type="text/javascript" src="../../../app/javascript/anim/anim.js"></script>
-    <script type="text/javascript" src="../../../app/javascript/lazr/lazr.js"></script>
-    <script type="text/javascript" src="../../../app/javascript/overlay/overlay.js"></script>
-    <script type="text/javascript" src="../../../app/javascript/picker/picker.js"></script>
-    <script type="text/javascript" src="../../../app/javascript/picker/person_picker.js"></script>
-    <script type="text/javascript" src="../../../app/javascript/picker/picker_patcher.js"></script>
-    <!-- The module under test -->
-    <script type="text/javascript" src="../distroseries.js"></script>
-    <!-- The test suite -->
-    <script type="text/javascript" src="test_distroseries.js"></script>
+  <!-- Required modules -->
+  <script type="text/javascript"
+          src="../../../app/javascript/client.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/activator/activator.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/anim/anim.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/lazr/lazr.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/overlay/overlay.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/picker/picker.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/picker/person_picker.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/picker/picker_patcher.js"></script>
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../distroseries.initseries.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_distroseries.initseries.js"></script>
+
   </head>
   <body class="yui3-skin-sam">
+    <ul id="suites">
+      <li>lp.registry.distroseries.initseries.test</li>
+    </ul>
   </body>
 </html>
 
=== renamed file 'lib/lp/registry/javascript/tests/test_distroseries.js' => 'lib/lp/registry/javascript/tests/test_distroseries.initseries.js'
--- lib/lp/registry/javascript/tests/test_distroseries.js	2011-07-08 05:39:39 +0000
+++ lib/lp/registry/javascript/tests/test_distroseries.initseries.js	2011-07-18 11:54:37 +0000
@@ -1,9 +1,16 @@
-/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
-
-YUI().use(
-        'lp.testing.runner', 'test', 'console', 'node-event-simulate',
-        'lp.registry.distroseries.initseries',
-        function(Y) {
+/**
+ * Copyright 2011 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Tests for DistroSeries related stuff.
+ *
+ * @module lp.registry.distroseries
+ * @submodule test
+ */
+
+YUI.add('lp.registry.distroseries.initseries.test', function(Y) {
+
+    var namespace = Y.namespace('lp.registry.distroseries.initseries.test');
 
     var Assert = Y.Assert;
     var ArrayAssert = Y.ArrayAssert;
@@ -440,6 +447,7 @@
         },
 
         testParentGetter: function() {
+            var parent;
             parent = {value: "4", title: "Hoary", api_uri: "ubuntu/hoary"};
             this.widget.add_parent(parent);
             parent = {value: "3", title: "Warty", api_uri: "ubuntu/warty"};
@@ -495,7 +503,7 @@
                     Assert.isFunction(config.on.failure);
                 }
             };
-            distroseries = {api_uri: "ubuntu/hoary", value: 3};
+            var distroseries = {api_uri: "ubuntu/hoary", value: 3};
             this.widget.add_distroseries(distroseries);
             Assert.isTrue(io, "No IO initiated.");
         },
@@ -1056,6 +1064,10 @@
                         else if (name === "overlay_components") {
                             return ['restricted', null];
                         }
+                        else {
+                            Assert.fail("Unrecognized property: " + name);
+                            return null; // Keep lint quiet.
+                        }
                     }
                 },
                 architectureChoice: {
@@ -1169,30 +1181,10 @@
             Y.one('body').appendChild(node);
         },
 
-        setUpFirstDerivation: function() {
-            window.LP = {cache: {is_first_derivation: true}};
-        },
-
-        setUpNotFirstDerivation: function() {
-            window.LP = {cache:
-                {is_first_derivation: false,
-                 previous_series: {
-                     api_uri: '/ubuntu/natty',
-                     value: '3',
-                     title: 'Ubunty: Natty'},
-                 previous_parents: [
-                     {api_uri: '/debian/sid',
-                      value: '4',
-                      title: 'Debian: Sid'},
-                     {api_uri: '/zz/aa',
-                      value: '5',
-                      title: 'ZZ: aa'}]}};
-        },
-
         testIsFirstDerivation: function() {
-            this.setUpFirstDerivation();
-            var form_actions = initseries.setupWidgets();
-            initseries.setupInteraction(form_actions);
+            var cache = {is_first_derivation: true};
+            var form_actions = initseries.setupWidgets(cache);
+            initseries.setupInteraction(form_actions, cache);
 
             // No pre-populated parent.
             ArrayAssert.itemsAreEqual(
@@ -1201,8 +1193,21 @@
         },
 
         testIsNotFirstDerivation: function() {
-            this.setUpNotFirstDerivation();
-            var form_actions = initseries.setupWidgets();
+            var cache = {
+                is_first_derivation: false,
+                previous_series: {
+                    api_uri: '/ubuntu/natty',
+                    value: '3',
+                    title: 'Ubunty: Natty'
+                },
+                previous_parents: [
+                    {api_uri: '/debian/sid',
+                     value: '4', title: 'Debian: Sid'},
+                    {api_uri: '/zz/aa',
+                     value: '5', title: 'ZZ: aa'}
+                ]
+            };
+            var form_actions = initseries.setupWidgets(cache);
             // Monkeypatch LP client.
             var client = {
                 get: function(path, config) {
@@ -1220,7 +1225,7 @@
                 }
             };
             form_actions.architectureChoice.client = client;
-            initseries.setupInteraction(form_actions);
+            initseries.setupInteraction(form_actions, cache);
 
             // Parents are populated.
             ArrayAssert.itemsAreEqual(
@@ -1237,7 +1242,8 @@
     };
 
     suite.add(new Y.Test.Case(testDeriveDistroSeriesSetup));
-
-    Y.lp.testing.Runner.run(suite);
-
-});
+    namespace.suite = suite;
+
+}, "0.1", {"requires": [
+               'test', 'console', 'node-event-simulate',
+               'lp.registry.distroseries.initseries']});