From 5c3aaacd2e5748b3f104f5ee033b442582d44c07 Mon Sep 17 00:00:00 2001 From: XavierDo Date: Wed, 6 Feb 2019 12:10:36 +0000 Subject: [PATCH] [IMP] mail: correctly patch mail status manager timers We want to avoid that a `setTimeout` started in one test has an impact on other tests. This is especially true with the status update loop. Since mail_service is used in multiple tests, the safest solution is to patch timeouts everytime we make a call to getMailServices. There is no need for an unpatch, the patch become the default behaviour until the page is refresh at the end of tests. It is possible to manually add the patch after calling `getMailServices` ``` this.services = mailTestUtils.getMailServices(this); this.timeoutMock = mailTestUtils.patchMailTimeouts(); ``` This can be usefull if we want to get the timeoutMock to simulate time changes. Task: 1856205 --- .../src/js/services/mail_status_manager.js | 27 +---- addons/mail/static/src/js/utils.js | 10 ++ .../mail/static/tests/helpers/test_utils.js | 108 ++++++++++++++++++ .../static/tests/mail_status_manager_tests.js | 66 +++-------- 4 files changed, 137 insertions(+), 74 deletions(-) diff --git a/addons/mail/static/src/js/services/mail_status_manager.js b/addons/mail/static/src/js/services/mail_status_manager.js index 2b75cdc6ee7..5ca151d47bf 100644 --- a/addons/mail/static/src/js/services/mail_status_manager.js +++ b/addons/mail/static/src/js/services/mail_status_manager.js @@ -3,6 +3,7 @@ odoo.define('mail.Manager.Status', function (require) { var core = require('web.core'); var MailManager = require('mail.Manager'); +var mailUtils = require('mail.utils'); var QWeb = core.qweb; /** @@ -33,8 +34,8 @@ MailManager.include({ // Add to list to call it in next bus update or _fetchMissingImStatus this._imStatus[partnerID] = undefined; // fetch after some time if no other getImStatus occurs - this._clearStatusServiceTimeout(this._fetchStatusTimeout); - this._fetchStatusTimeout = this._setStatusServiceTimeout(function () { + mailUtils.clearTimeout(this._fetchStatusTimeout); + this._fetchStatusTimeout = mailUtils.setTimeout(function () { self._fetchMissingImStatus(); }, 500); } @@ -67,15 +68,6 @@ MailManager.include({ // Private //-------------------------------------------------------------------------- - /** - * A simple clearTimeout, useful for test - * - * @private - * @param {integer} ids - */ - _clearStatusServiceTimeout: function (id) { - clearTimeout(id); - }, /** * Fetch the list of im_status for partner with id in ids list and triggers * an update. @@ -168,17 +160,6 @@ MailManager.include({ } }); }, - /** - * A simple setTimeout, useful for test - * - * @private - * @param {function} func - * @param {integer} duration - * @return {integer} - */ - _setStatusServiceTimeout: function (func, duration) { - return setTimeout(func, duration); - }, /** * Once initialised, this loop will update the im_status of registered * users. @@ -191,7 +172,7 @@ MailManager.include({ if (!_.isNumber(counter)) { counter = 0; } - this._setStatusServiceTimeout(function () { + mailUtils.setTimeout(function () { if (counter >= self._UPDATE_INTERVAL && self._isTabFocused) { self._fetchImStatus({ partnerIDs: self._getImStatusToUpdate() }); counter = 0; diff --git a/addons/mail/static/src/js/utils.js b/addons/mail/static/src/js/utils.js index 1f31a6ead2f..14ac97def76 100644 --- a/addons/mail/static/src/js/utils.js +++ b/addons/mail/static/src/js/utils.js @@ -91,6 +91,14 @@ function timeFromNow(date) { return date.fromNow(); } +function o_clearTimeout(id) { + return clearTimeout(id); +} + +function o_setTimeout(func, delay) { + return setTimeout(func, delay); +} + return { addLink: addLink, getTextToHTML: getTextToHTML, @@ -100,6 +108,8 @@ return { parseEmail: parseEmail, stripHTML: stripHTML, timeFromNow: timeFromNow, + clearTimeout: o_clearTimeout, + setTimeout: o_setTimeout, }; }); diff --git a/addons/mail/static/tests/helpers/test_utils.js b/addons/mail/static/tests/helpers/test_utils.js index cf63c281c58..063e3a9eb03 100644 --- a/addons/mail/static/tests/helpers/test_utils.js +++ b/addons/mail/static/tests/helpers/test_utils.js @@ -5,6 +5,7 @@ var BusService = require('bus.BusService'); var Discuss = require('mail.Discuss'); var MailService = require('mail.Service'); +var mailUtils = require('mail.utils'); var AbstractStorageService = require('web.AbstractStorageService'); var Class = require('web.Class'); @@ -79,6 +80,111 @@ var MockMailService = Class.extend({ }, }); +/** + * Patch all the mailUtils.clearTimeout and mailUtils.setTimeout. + * + * @return {Object} helper functions, including unpatch and time management tools. + */ +var patchMailTimeouts = function () { + var currentTime = 0; + var timeouts = {}; + var countTimeout = 0; + + mailUtils.clearTimeout = function (id) { + delete timeouts[id]; + }; + + mailUtils.setTimeout = function (func, duration) { + duration = duration || 0; + var executeTime = currentTime + duration; + countTimeout++; + timeouts[countTimeout] = { + executeTime: executeTime, + func: func + }; + return countTimeout; + }; + /** + * @return {integer|boolean} id of the next timeout in queue, false if queue is empty + */ + function getNextTimeoutId() { + var minKey = false; + _.each(timeouts, function (value, key) { + if (minKey === false) { + minKey = Number(key); + return; + } + var minTime = timeouts[minKey].executeTime; + if (value.executeTime < minTime || (value.executeTime === minTime && key < minKey)) { + minKey = Number(key); + } + }); + return minKey; + } + + /** + * @return {integer|boolean} delay (time interval) before the next timeout in queue is executed. + * Useful to know how much time to advance to execute next timer. + */ + function getNextTimeoutDelay() { + var next = getNextTimeoutId(); + if (next === false) { + return false; + } + return timeouts[next].executeTime - currentTime; + } + + /** + * Set the current time to given time + * + * @param {integer} time + */ + function setTime(time) { + var next = getNextTimeoutId(); + if (next !== false && timeouts[next].executeTime <= time) { + currentTime = timeouts[next].executeTime; + var func = timeouts[next].func; + // watch out setTimeout inside setTimeout (recursive) + delete timeouts[next]; + func(); + setTime(time); + } + else { + currentTime = time; + } + } + + /** + * Add the given time to current time + * + * @param {integer} time + */ + function addTime(time) { + setTime(currentTime + time); + } + + /** + * Set time to the max time in queue and execute all timeouts before this time. + */ + function runPendingTimeouts() { + var maxTimeInQueue = 0; + _.each(timeouts, function (value, key) { + if (value.executeTime > maxTimeInQueue) { + maxTimeInQueue = value.executeTime; + } + }); + setTime(maxTimeInQueue); + } + + return { + addTime: addTime, + getNextTimeoutDelay:getNextTimeoutDelay, + runPendingTimeouts: runPendingTimeouts, + setTime: setTime, + }; +}; + + /** * Returns the list of mail services required by the mail components: a * mail_service, and its two dependencies bus_service and local_storage. @@ -87,6 +193,7 @@ var MockMailService = Class.extend({ * and local_storage, in that order */ function getMailServices() { + patchMailTimeouts(); return new MockMailService().getServices(); } @@ -94,6 +201,7 @@ return { MockMailService: MockMailService, createDiscuss: createDiscuss, getMailServices: getMailServices, + patchMailTimeouts: patchMailTimeouts, }; }); diff --git a/addons/mail/static/tests/mail_status_manager_tests.js b/addons/mail/static/tests/mail_status_manager_tests.js index f2056bf2280..a1ae329c9dc 100644 --- a/addons/mail/static/tests/mail_status_manager_tests.js +++ b/addons/mail/static/tests/mail_status_manager_tests.js @@ -13,42 +13,12 @@ QUnit.module('mail', {}, function () { QUnit.module('service', {}, function () { QUnit.module('Status manager', { beforeEach: function () { - var self = this; - this.services = mailTestUtils.getMailServices(); - this.timeouts = {}; - this.countTimeout = 0; - this.defaultPatchData = { - _clearStatusServiceTimeout: function (id) { - self.timeouts[id] = false; - }, - _setStatusServiceTimeout: function (func, duration) { - var id = self.countTimeout; - self.timeouts[id] = func; - self.countTimeout++; - return id; - }, - _updateImStatusLoop: function () {}, // avoid spam and infinite loop - }; - this.patchMailService = function (patch) { - testUtils.mock.patch(this.services.mail_service, _.extend({}, this.defaultPatchData, patch)); - }; - this.resolveTimeouts = function () { - var timeouts = _.extend({}, self.timeouts); - self.timeouts = {}; // empty timeout before looping to avoid to remove loop timeout - _.each(_.values(timeouts), function (func) { - if (func !== false) { - func(); - } - }); - }; - }, - afterEach: function () { - testUtils.mock.unpatch(this.services.mail_service); + this.services = mailTestUtils.getMailServices(this); + this.timeoutMock = mailTestUtils.patchMailTimeouts(); }, }); QUnit.test('simple set im_status', function (assert) { assert.expect(1); - this.patchMailService(); var parent = testUtils.createParent({ services: this.services, mockRPC: function (route, args) { @@ -64,13 +34,12 @@ QUnit.test('simple set im_status', function (assert) { }]); assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 1 }), 'online'); - this.resolveTimeouts(); // alway resolve timeout + this.timeoutMock.runPendingTimeouts(); parent.destroy(); }); QUnit.test('multi get_im_status', function (assert) { assert.expect(9); - this.patchMailService(); var readCount = 0; var parent = testUtils.createParent({ //data: this.data, @@ -99,21 +68,19 @@ QUnit.test('multi get_im_status', function (assert) { assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 2 }), undefined); assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 3 }), undefined); - this.resolveTimeouts(); + this.timeoutMock.runPendingTimeouts(); assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 1 }), 'online'); assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 2 }), 'away'); assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 3 }), 'im_partner'); - this.resolveTimeouts(); + this.timeoutMock.runPendingTimeouts(); assert.strictEqual(readCount, 1, 'Only one read on partner should have been performed'); parent.destroy(); }); QUnit.test('update loop', function (assert) { - assert.expect(11); - delete this.defaultPatchData['_updateImStatusLoop']; // we want to test default behaviour of loop - this.patchMailService(); + assert.expect(12); var readCount = 0; var parent = testUtils.createParent({ services: this.services, @@ -140,13 +107,10 @@ QUnit.test('update loop', function (assert) { { id: 3, im_status: 'im_partner' }, //shouldn't be updated !!!! ]); //_updateImStatusLoop should be running at one second per iteration, lets make a minute pass. - // this test is strongly dependant on the fact that the update loop as 1 second tick and update every 50 seconds. assert.strictEqual(readCount, 0); - for (var i = 0; i < 50; i++){ - this.resolveTimeouts(); - } + this.timeoutMock.addTime(50*1000); assert.strictEqual(readCount, 0); - this.resolveTimeouts(); //one more second pass + this.timeoutMock.addTime(1000); assert.strictEqual(readCount, 1, 'one call should have been made after 50 seconds' ); assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 1 }), 'online'); assert.strictEqual(parent.call('mail_service', 'getImStatus', { partnerID: 2 }), 'away'); @@ -154,16 +118,17 @@ QUnit.test('update loop', function (assert) { //simulate change of focus //original listener: $(window).on("blur", this._onWindowFocusChange.bind(this, false); + unload, ... parent.call('mail_service', '_onWindowFocusChange', false); // remove focus from tab - for (var i = 0; i < 200; i++){ // a lot of time has passed - this.resolveTimeouts(); - } - assert.strictEqual(readCount, 1, 'No more call should have been performed'); + this.timeoutMock.addTime(5*60*1000); // x minutes without focus, no rpc should be done during this time + assert.strictEqual(readCount, 1, 'No more call should have been performed'); //simulate change of focus //original listener: $(window).on("focus", this._onWindowFocusChange.bind(this, true); parent.call('mail_service', '_onWindowFocusChange', true); // give focus to tab - this.resolveTimeouts(); //one more second pass + var nextUpdateDelay = this.timeoutMock.getNextTimeoutDelay(); + assert.strictEqual(nextUpdateDelay, 1000, "next update should be done in maximum one second"); + this.timeoutMock.addTime(nextUpdateDelay); // one second should be enough assert.strictEqual(readCount, 2, 'One more call should have been done once tab focused'); + this.timeoutMock.runPendingTimeouts(); parent.destroy(); }); @@ -171,7 +136,6 @@ QUnit.test('update status', function (assert) { // the current solution to look for updatable im_status in dom is not perfect, but waiting for // ability to include widgets in views, this is the most simple solution assert.expect(2); - this.patchMailService(); var StatusWidget = Widget.extend({ start: function () { this.render(); @@ -204,7 +168,7 @@ QUnit.test('update status', function (assert) { { id: 1, im_status: 'offline' }, ]); assert.notOk(statusWidget.$('.o_updatable_im_status i').hasClass('o_user_online')); - this.resolveTimeouts(); + this.timeoutMock.runPendingTimeouts(); statusWidget.destroy(); });