launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08420
[Merge] lp:~frankban/launchpad/bug-996720 into lp:launchpad
Francesco Banconi has proposed merging lp:~frankban/launchpad/bug-996720 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #996720 in Launchpad itself: "lib/lp/soyuz/javascript/tests/test_lp_dynamic_dom_updater.html fails rarely/intermittently in parallel tests"
https://bugs.launchpad.net/launchpad/+bug/996720
For more details, see:
https://code.launchpad.net/~frankban/launchpad/bug-996720/+merge/108196
= 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
--
https://code.launchpad.net/~frankban/launchpad/bug-996720/+merge/108196
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~frankban/launchpad/bug-996720 into lp:launchpad.
=== 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.");
Follow ups
