Merge lp:~frankban/launchpad/bug-996720 into lp:launchpad

[Francesco Banconi]

The bug: lib/lp/soyuz/javascript/tests/test_lp_dynamic_dom_updater.html fails rarely/intermittently in parallel tests.

= Summary =

lib/lp/soyuz/javascript/tests/test_lp_dynamic_dom_updater.html fails rarely/intermittently in parallel tests.
This doesn't seem a test ordering or isolation issue, and it's not possible to reproduce the failure using the worker list attached to the bug. This seems to be a timing issue produced when the test is run in a heavy loaded machine, like the ec2 instances we use in parallel tests.

== Proposed fix ==

Many of the tests in lib/lp/soyuz/javascript/tests/lp_dynamic_dom_updater.js assume the test runner to be fast, and the environment to be responsive. We should either change the code to be able to mock times, or create a separate/testable function that handles polling intervals.

== Pre-implementation notes ==

Benji gave me a hint about the direction to follow.

== Implementation details ==

lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js
Created a separate function that updates the polling interval, given the elapsed time.

lib/lp/soyuz/javascript/tests/lp_dynamic_dom_updater.js
The new function is now directly tested, and we can pass arbitrary elapsed times without having to mock _request_start.
s/constants/parameters in all the relevant tests.

== Tests ==

$ xvfb-run bin/test -cvvt lib/lp/soyuz/javascript/tests/test_lp_dynamic_dom_updater.html

== Demo and Q/A ==

no qa

[j.c.sackett (jcsackett)]

This looks fine to land.

review: Approve

=== modified file 'lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js'
--- lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js	2011-04-06 00:35:12 +0000
+++ lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js	2012-05-31 16:12:34 +0000
@@ -282,6 +282,38 @@
         },
 
         /**
+         * Update our actual poll interval depending on the elapsed time
+         * for this request.
+         *
+         * @method _updateActualInterval
+         * @private
+         */
+        _updateActualInterval: function(elapsed_time) {
+            if (elapsed_time > this.get('long_processing_time')) {
+                this._actual_interval *= 2;
+                return true;
+            }
+            if (elapsed_time < this.get('short_processing_time')) {
+                var new_actual_interval = this._actual_interval / 2;
+
+                // If the newly-calculated actual interval is greater than
+                // the config interval then we update to the new interval,
+                // otherwise if the actual interval is currently greater
+                // than the config interval, then we update to the config
+                // interval.
+                var config_interval = this.get('interval');
+                if (new_actual_interval >= config_interval) {
+                    this._actual_interval = new_actual_interval;
+                    return true;
+                } else if (this._actual_interval > config_interval){
+                    this._actual_interval = config_interval;
+                    return true;
+                }
+            }
+            return false;
+        },
+
+        /**
          * Success handler for the call to the LP API.
          *
          * @method _handleSuccess
@@ -301,30 +333,9 @@
             // subtree with the returned data.
             this.update(data);
 
-            // Update our actual poll interval depending on the elapsed time
-            // for this request:
-            var actual_interval_updated = false;
-            if (elapsed_time > this.get('long_processing_time')) {
-                this._actual_interval *= 2;
-                actual_interval_updated = true;
-            } else if (elapsed_time < this.get('short_processing_time')) {
-                var new_actual_interval = this._actual_interval / 2;
-
-                // If the newly-calculated actual interval is greater than
-                // the config interval then we update to the new interval,
-                // otherwise if the actual interval is currently greater
-                // than the config interval, then we update to the config
-                // interval.
-                var config_interval = this.get('interval');
-                if (new_actual_interval >= config_interval) {
-                    this._actual_interval = new_actual_interval;
-                    actual_interval_updated = true;
-                } else if (this._actual_interval > config_interval){
-                    this._actual_interval = config_interval;
-                    actual_interval_updated = true;
-                }
-            }
-
+            // Update our actual poll interval.
+            var actual_interval_updated = this._updateActualInterval(
+                elapsed_time)
             if (actual_interval_updated) {
                 Y.log("Actual poll interval updated to " +
                     this._actual_interval + "ms.");
@@ -334,7 +345,6 @@
             setTimeout(
                 Y.bind(this.dynamicUpdate, this),
                 this._actual_interval);
-
         },
 
         /**

=== modified file 'lib/lp/soyuz/javascript/tests/lp_dynamic_dom_updater.js'
--- lib/lp/soyuz/javascript/tests/lp_dynamic_dom_updater.js	2011-06-15 21:23:28 +0000
+++ lib/lp/soyuz/javascript/tests/lp_dynamic_dom_updater.js	2012-05-31 16:12:34 +0000
@@ -163,15 +163,17 @@
         // Wait 5ms just to ensure that the DynamicDomUpdater finishes
         // its initialization.
         this.wait(function() {
-            // Before calling the success handler, ensure that the
-            // elapsed time will be evaluated as greater than the
-            // long_processing_time defined in the config.
-            this.eg_div.updater._request_start = new Date().getTime() - 11;
-            this.eg_div.updater._handleSuccess({msg: "Boo. I've changed."});
+            var updated = this.eg_div.updater._updateActualInterval(
+                this.config.long_processing_time + 1);
+
+            // The actual interval is updated.
+            Assert.isTrue(
+                updated,
+                "Poll interval changed if request takes too long.");
 
             // The actual interval should have doubled from 1ms to 2ms.
             Assert.areEqual(
-                2,
+                this.config.interval * 2,
                 this.eg_div.updater._actual_interval,
                 "Poll interval doubled if request takes too long.");
 
@@ -188,16 +190,18 @@
         // Wait 5ms just to ensure that the DynamicDomUpdater finishes
         // its initialization.
         this.wait(function() {
-            // Before calling the success handler, ensure that the
-            // elapsed time will be evaluated as greater than the
-            // short_processing_time but less than the long_processing_time
-            // defined in the config.
-            this.eg_div.updater._request_start = new Date().getTime() - 7;
-            this.eg_div.updater._handleSuccess({msg: "Boo. I've changed."});
+            var updated = this.eg_div.updater._updateActualInterval(
+               (this.config.long_processing_time +
+                this.config.short_processing_time) / 2);
+
+            // The actual interval is not updated.
+            Assert.isFalse(
+                updated,
+                "Poll interval unchanged if request is timely.");
 
             // The actual interval is unchanged.
             Assert.areEqual(
-                1,
+                this.config.interval,
                 this.eg_div.updater._actual_interval,
                 "Poll interval unchanged if request is timely.");
 
@@ -207,27 +211,28 @@
     test_actual_interval_halved_if_request_is_quick: function() {
         // The actual polling interval is halved if the elapsed time
         // for the previous request was less than the short_processing_time
-
         this.eg_div.plug(
             Y.lp.soyuz.dynamic_dom_updater.DynamicDomUpdater, this.config);
 
         // Wait 5ms just to ensure that the DynamicDomUpdater finishes
         // its initialization.
         this.wait(function() {
-            // Before calling the success handler, ensure that the
-            // elapsed time will be evaluated as less than the
-            // short_processing_time defined in the config.
-            this.eg_div.updater._request_start = new Date().getTime();
 
             // Ensure the current actual polling time is more than double the
             // requested interval.
-            this.eg_div.updater._actual_interval = 16;
-
-            this.eg_div.updater._handleSuccess({msg: "Boo. I've changed."});
-
-            // The actual interval is unchanged.
+            var actual_interval = this.config.interval * 4;
+            this.eg_div.updater._actual_interval = actual_interval;
+            var updated = this.eg_div.updater._updateActualInterval(
+               this.config.short_processing_time - 1);
+
+            // The actual interval is updated.
+            Assert.isTrue(
+                updated,
+                "Poll interval changed if request is faster than expected.");
+
+            // The actual interval is changed.
             Assert.areEqual(
-                8,
+                actual_interval / 2,
                 this.eg_div.updater._actual_interval,
                 "Poll interval is halved if request is faster than " +
                 "expected.");
@@ -245,23 +250,26 @@
         // Wait 5ms just to ensure that the DynamicDomUpdater finishes
         // its initialization.
         this.wait(function() {
-            // Before calling the success handler, ensure that the
-            // elapsed time will be evaluated as less than the
-            // short_processing_time defined in the config.
-            this.eg_div.updater._request_start = new Date().getTime();
-
-            // Ensure the current actual polling time is more than double the
+            var interval = 10;
+
+            // Update the configuration interval:
+            this.eg_div.updater.set('interval', interval);
+
+            // Ensure the current actual polling time is less than double the
             // requested interval.
-            this.eg_div.updater._actual_interval = 16;
-
-            // Ensure that the configuration interval is greater than 8:
-            this.eg_div.updater.set('interval', 10);
-
-            this.eg_div.updater._handleSuccess({msg: "Boo. I've changed."});
-
-            // The actual interval is unchanged.
+            this.eg_div.updater._actual_interval = (interval * 2) - 2;
+            var updated = this.eg_div.updater._updateActualInterval(
+               this.config.short_processing_time - 1);
+
+            // The actual interval is updated.
+            Assert.isTrue(
+                updated,
+                "Poll interval changed if request is faster than expected.");
+
+            // The actual interval is not smaller than the configuration
+            // interval.
             Assert.areEqual(
-                10,
+                interval,
                 this.eg_div.updater._actual_interval,
                 "Actual interval never goes below config interval.");