From cd1cf09dc98af832bcbcdc40da84523ecd53faa7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 31 Jan 2017 22:40:53 +0000 Subject: [PATCH] Make tests pass on Chrome again It seems that a number of the tests had started failing when run in Chrome. They were fine under PhantomJS, but the MegolmExport tests only work under Chrome, and I need them to work... Mostly the problems were timing-related, where assumptions made about how quickly the `then` handler on a promise would be called were no longer valid. Possibly Chrome 55 has made some changes to the relative priorities of setTimeout and sendMessage calls. One of the TimelinePanel tests was failing because it was expecting the contents of a div to take up more room than they actually were. It's possible this is something very environment-specific; hopefully the new value will work on a wider range of machines. Also some logging tweaks. --- karma.conf.js | 8 +++++ src/components/structures/ScrollPanel.js | 10 +++---- test/components/structures/RoomView-test.js | 13 +++----- .../components/structures/ScrollPanel-test.js | 30 +++++++++++++++---- .../structures/TimelinePanel-test.js | 8 +++-- test/test-utils.js | 8 +++++ 6 files changed, 56 insertions(+), 21 deletions(-) diff --git a/karma.conf.js b/karma.conf.js index 131a03ce79..6d3047bb3b 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -165,6 +165,14 @@ module.exports = function (config) { }, devtool: 'inline-source-map', }, + + webpackMiddleware: { + stats: { + // don't fill the console up with a mahoosive list of modules + chunks: false, + }, + }, + browserNoActivityTimeout: 15000, }); }; diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 1391d2b740..c6bcdc45cd 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -570,7 +570,7 @@ module.exports = React.createClass({ var boundingRect = node.getBoundingClientRect(); var scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom; - debuglog("Scrolling to token '" + node.dataset.scrollToken + "'+" + + debuglog("ScrollPanel: scrolling to token '" + node.dataset.scrollToken + "'+" + pixelOffset + " (delta: "+scrollDelta+")"); if(scrollDelta != 0) { @@ -582,7 +582,7 @@ module.exports = React.createClass({ _saveScrollState: function() { if (this.props.stickyBottom && this.isAtBottom()) { this.scrollState = { stuckAtBottom: true }; - debuglog("Saved scroll state", this.scrollState); + debuglog("ScrollPanel: Saved scroll state", this.scrollState); return; } @@ -601,12 +601,12 @@ module.exports = React.createClass({ trackedScrollToken: node.dataset.scrollToken, pixelOffset: wrapperRect.bottom - boundingRect.bottom, }; - debuglog("Saved scroll state", this.scrollState); + debuglog("ScrollPanel: saved scroll state", this.scrollState); return; } } - debuglog("Unable to save scroll state: found no children in the viewport"); + debuglog("ScrollPanel: unable to save scroll state: found no children in the viewport"); }, _restoreSavedScrollState: function() { @@ -640,7 +640,7 @@ module.exports = React.createClass({ this._lastSetScroll = scrollNode.scrollTop; } - debuglog("Set scrollTop:", scrollNode.scrollTop, + debuglog("ScrollPanel: set scrollTop:", scrollNode.scrollTop, "requested:", scrollTop, "_lastSetScroll:", this._lastSetScroll); }, diff --git a/test/components/structures/RoomView-test.js b/test/components/structures/RoomView-test.js index 58db29b1ee..8e7c8160b8 100644 --- a/test/components/structures/RoomView-test.js +++ b/test/components/structures/RoomView-test.js @@ -42,17 +42,12 @@ describe('RoomView', function () { it('resolves a room alias to a room id', function (done) { peg.get().getRoomIdForAlias.returns(q({room_id: "!randomcharacters:aser.ver"})); - var onRoomIdResolved = sinon.spy(); + function onRoomIdResolved(room_id) { + expect(room_id).toEqual("!randomcharacters:aser.ver"); + done(); + } ReactDOM.render(, parentDiv); - - process.nextTick(function() { - // These expect()s don't read very well and don't give very good failure - // messages, but expect's toHaveBeenCalled only takes an expect spy object, - // not a sinon spy object. - expect(onRoomIdResolved.called).toExist(); - done(); - }); }); it('joins by alias if given an alias', function (done) { diff --git a/test/components/structures/ScrollPanel-test.js b/test/components/structures/ScrollPanel-test.js index 13721c9ecd..eacaeb5fb4 100644 --- a/test/components/structures/ScrollPanel-test.js +++ b/test/components/structures/ScrollPanel-test.js @@ -73,6 +73,7 @@ var Tester = React.createClass({ /* returns a promise which will resolve when the fill happens */ awaitFill: function(dir) { + console.log("ScrollPanel Tester: awaiting " + dir + " fill"); var defer = q.defer(); this._fillDefers[dir] = defer; return defer.promise; @@ -80,7 +81,7 @@ var Tester = React.createClass({ _onScroll: function(ev) { var st = ev.target.scrollTop; - console.log("Scroll event; scrollTop: " + st); + console.log("ScrollPanel Tester: scroll event; scrollTop: " + st); this.lastScrollEvent = st; var d = this._scrollDefer; @@ -159,10 +160,29 @@ describe('ScrollPanel', function() { scrollingDiv = ReactTestUtils.findRenderedDOMComponentWithClass( tester, "gm-scroll-view"); - // wait for a browser tick to let the initial paginates complete - setTimeout(function() { - done(); - }, 0); + // we need to make sure we don't call done() until q has finished + // running the completion handlers from the fill requests. We can't + // just use .done(), because that will end up ahead of those handlers + // in the queue. We can't use window.setTimeout(0), because that also might + // run ahead of those handlers. + const sp = tester.scrollPanel(); + let retriesRemaining = 1; + const awaitReady = function() { + return q().then(() => { + if (sp._pendingFillRequests.b === false && + sp._pendingFillRequests.f === false + ) { + return; + } + + if (retriesRemaining == 0) { + throw new Error("fillRequests did not complete"); + } + retriesRemaining--; + return awaitReady(); + }); + }; + awaitReady().done(done); }); afterEach(function() { diff --git a/test/components/structures/TimelinePanel-test.js b/test/components/structures/TimelinePanel-test.js index b2cdfbd590..be60691b5c 100644 --- a/test/components/structures/TimelinePanel-test.js +++ b/test/components/structures/TimelinePanel-test.js @@ -99,7 +99,11 @@ describe('TimelinePanel', function() { // the document so that we can interact with it properly. parentDiv = document.createElement('div'); parentDiv.style.width = '800px'; - parentDiv.style.height = '600px'; + + // This has to be slightly carefully chosen. We expect to have to do + // exactly one pagination to fill it. + parentDiv.style.height = '500px'; + parentDiv.style.overflow = 'hidden'; document.body.appendChild(parentDiv); }); @@ -235,7 +239,7 @@ describe('TimelinePanel', function() { expect(client.paginateEventTimeline.callCount).toEqual(0); done(); }, 0); - }, 0); + }, 10); }); it("should let you scroll down to the bottom after you've scrolled up", function(done) { diff --git a/test/test-utils.js b/test/test-utils.js index cdfae4421c..71d3bd92d6 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -14,7 +14,15 @@ var MatrixEvent = jssdk.MatrixEvent; */ export function beforeEach(context) { var desc = context.currentTest.fullTitle(); + console.log(); + + // this puts a mark in the chrome devtools timeline, which can help + // figure out what's been going on. + if (console.timeStamp) { + console.timeStamp(desc); + } + console.log(desc); console.log(new Array(1 + desc.length).join("=")); };