Skip to content

Commit

Permalink
Fix some races in the tests
Browse files Browse the repository at this point in the history 
There is a common pattern in the tests which is, when we want to mock a /sync,
to flush it, and then, in the next tick of the promise loop, to wait for the
syncing event. However, this is racy: there is no guarantee that the syncing
event will not happen before the next tick of the promise loop.

Instead, we should set the expectation of the syncing event, then do the flush.
(Technically we only need to wait for the syncing event, but by waiting for
both we'll catch any errors thrown by the flush, and make sure we don't have
any outstanding flushes before proceeding).

Add a utility method to TestClient to do the above, and use it where we have a
TestClient.

(Also fixes a couple of other minor buglets in the tests).
  • Loading branch information
@richvdh
richvdh committed Jul 4, 2017
1 parent cc16cb9 commit ae950a2
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 118 deletions.
13 changes: 13 additions & 0 deletions spec/TestClient.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,16 @@ TestClient.prototype.getSigningKey = function() {
const keyId = 'ed25519:' + this.deviceId;
return this.deviceKeys.keys[keyId];
};

/**
* flush a single /sync request, and wait for the syncing event
*
* @returns {Promise} promise which completes once the sync has been flushed
*/
TestClient.prototype.flushSync = function() {
console.log(`${this}: flushSync`);
return q.all([
this.httpBackend.flush('/sync', 1),
testUtils.syncPromise(this.client),
]);
};
8 changes: 5 additions & 3 deletions spec/integ/matrix-client-crypto.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ limitations under the License.

"use strict";
import 'source-map-support/register';

// load olm before the sdk if possible
import '../olm-loader';

import expect from 'expect';
const sdk = require("../..");
const q = require("q");
Expand Down Expand Up @@ -375,9 +379,7 @@ function firstSync(testClient) {
};

testClient.httpBackend.when("GET", "/sync").respond(200, syncData);
return testClient.httpBackend.flush("/sync", 1).then(() => {
return testUtils.syncPromise(testClient.client);
});
return testClient.flushSync();
}


Expand Down
14 changes: 8 additions & 6 deletions spec/integ/matrix-client-event-timeline.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -641,9 +641,10 @@ describe("MatrixClient event timelines", function() {
expect(tl.getEvents()[1].getContent().body).toEqual("a body");

// now let the sync complete, and check it again
return httpBackend.flush("/sync", 1);
}).then(function() {
return utils.syncPromise(client);
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]);
}).then(function() {
return client.getEventTimeline(timelineSet, event.event_id);
}).then(function(tl) {
Expand All @@ -670,9 +671,10 @@ describe("MatrixClient event timelines", function() {
expect(tl.getEvents()[1].getContent().body).toEqual("a body");
}).catch(utils.failTest).done(done);

httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).then(function() {
q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(function() {
return client.getEventTimeline(timelineSet, event.event_id);
}).then(function(tl) {
console.log("getEventTimeline completed (1)");
Expand Down
8 changes: 5 additions & 3 deletions spec/integ/matrix-client-opts.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const HttpBackend = require("../mock-request");
const utils = require("../test-utils");

import expect from 'expect';
import q from 'q';

describe("MatrixClient opts", function() {
const baseUrl = "http://localhost.or.something";
Expand Down Expand Up @@ -113,9 +114,10 @@ describe("MatrixClient opts", function() {
httpBackend.flush("/pushrules", 1).then(function() {
return httpBackend.flush("/filter", 1);
}).then(function() {
return httpBackend.flush("/sync", 1);
}).then(function() {
return utils.syncPromise(client);
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]);
}).done(function() {
expect(expectedEventTypes.length).toEqual(
0, "Expected to see event types: " + expectedEventTypes,
Expand Down
131 changes: 65 additions & 66 deletions spec/integ/matrix-client-room-timeline.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const EventStatus = sdk.EventStatus;
const HttpBackend = require("../mock-request");
const utils = require("../test-utils");

import q from 'q';
import expect from 'expect';

describe("MatrixClient room timelines", function() {
Expand Down Expand Up @@ -386,17 +387,17 @@ describe("MatrixClient room timelines", function() {
});

describe("new events", function() {
it("should be added to the right place in the timeline", function(done) {
it("should be added to the right place in the timeline", function() {
const eventData = [
utils.mkMessage({user: userId, room: roomId}),
utils.mkMessage({user: userId, room: roomId}),
];
setNextSyncData(eventData);

client.on("sync", function(state) {
if (state !== "PREPARED") {
return;
}
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(() => {
const room = client.getRoom(roomId);

let index = 0;
Expand All @@ -408,9 +409,10 @@ describe("MatrixClient room timelines", function() {
});

httpBackend.flush("/messages", 1);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).then(function() {
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(function() {
expect(index).toEqual(2);
expect(room.timeline.length).toEqual(3);
expect(room.timeline[2].event).toEqual(
Expand All @@ -419,12 +421,11 @@ describe("MatrixClient room timelines", function() {
expect(room.timeline[1].event).toEqual(
eventData[0],
);
}).catch(utils.failTest).done(done);
});
});
httpBackend.flush("/sync", 1);
});

it("should set the right event.sender values", function(done) {
it("should set the right event.sender values", function() {
const eventData = [
utils.mkMessage({user: userId, room: roomId}),
utils.mkMembership({
Expand All @@ -435,24 +436,24 @@ describe("MatrixClient room timelines", function() {
eventData[1].__prev_event = USER_MEMBERSHIP_EVENT;
setNextSyncData(eventData);

client.on("sync", function(state) {
if (state !== "PREPARED") {
return;
}
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(() => {
const room = client.getRoom(roomId);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).then(function() {
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(function() {
const preNameEvent = room.timeline[room.timeline.length - 3];
const postNameEvent = room.timeline[room.timeline.length - 1];
expect(preNameEvent.sender.name).toEqual(userName);
expect(postNameEvent.sender.name).toEqual("New Name");
}).catch(utils.failTest).done(done);
});
});
httpBackend.flush("/sync", 1);
});

it("should set the right room.name", function(done) {
it("should set the right room.name", function() {
const secondRoomNameEvent = utils.mkEvent({
user: userId, room: roomId, type: "m.room.name", content: {
name: "Room 2",
Expand All @@ -461,19 +462,20 @@ describe("MatrixClient room timelines", function() {
secondRoomNameEvent.__prev_event = ROOM_NAME_EVENT;
setNextSyncData([secondRoomNameEvent]);

client.on("sync", function(state) {
if (state !== "PREPARED") {
return;
}
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(() => {
const room = client.getRoom(roomId);
let nameEmitCount = 0;
client.on("Room.name", function(rm) {
nameEmitCount += 1;
});

httpBackend.flush("/sync", 1).then(() => {
return utils.syncPromise(client);
}).done(function() {
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(function() {
expect(nameEmitCount).toEqual(1);
expect(room.name).toEqual("Room 2");
// do another round
Expand All @@ -485,19 +487,18 @@ describe("MatrixClient room timelines", function() {
thirdRoomNameEvent.__prev_event = secondRoomNameEvent;
setNextSyncData([thirdRoomNameEvent]);
httpBackend.when("GET", "/sync").respond(200, NEXT_SYNC_DATA);
httpBackend.flush("/sync", 1).then(() => {
return utils.syncPromise(client);
}).done(function() {
expect(nameEmitCount).toEqual(2);
expect(room.name).toEqual("Room 3");
done();
});
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]);
}).then(function() {
expect(nameEmitCount).toEqual(2);
expect(room.name).toEqual("Room 3");
});
});
httpBackend.flush("/sync", 1);
});

it("should set the right room members", function(done) {
it("should set the right room members", function() {
const userC = "@cee:bar";
const userD = "@dee:bar";
const eventData = [
Expand All @@ -512,14 +513,15 @@ describe("MatrixClient room timelines", function() {
eventData[1].__prev_event = null;
setNextSyncData(eventData);

client.on("sync", function(state) {
if (state !== "PREPARED") {
return;
}
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(() => {
const room = client.getRoom(roomId);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).then(function() {
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(function() {
expect(room.currentState.getMembers().length).toEqual(4);
expect(room.currentState.getMember(userC).name).toEqual("C");
expect(room.currentState.getMember(userC).membership).toEqual(
Expand All @@ -529,30 +531,30 @@ describe("MatrixClient room timelines", function() {
expect(room.currentState.getMember(userD).membership).toEqual(
"invite",
);
}).catch(utils.failTest).done(done);
});
});
httpBackend.flush("/sync", 1);
});
});

describe("gappy sync", function() {
it("should copy the last known state to the new timeline", function(done) {
it("should copy the last known state to the new timeline", function() {
const eventData = [
utils.mkMessage({user: userId, room: roomId}),
];
setNextSyncData(eventData);
NEXT_SYNC_DATA.rooms.join[roomId].timeline.limited = true;

client.on("sync", function(state) {
if (state !== "PREPARED") {
return;
}
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(() => {
const room = client.getRoom(roomId);

httpBackend.flush("/messages", 1);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).done(function() {
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(function() {
expect(room.timeline.length).toEqual(1);
expect(room.timeline[0].event).toEqual(eventData[0]);
expect(room.currentState.getMembers().length).toEqual(2);
Expand All @@ -564,23 +566,21 @@ describe("MatrixClient room timelines", function() {
expect(room.currentState.getMember(otherUserId).membership).toEqual(
"join",
);
done();
});
});
httpBackend.flush("/sync", 1);
});

it("should emit a 'Room.timelineReset' event", function(done) {
it("should emit a 'Room.timelineReset' event", function() {
const eventData = [
utils.mkMessage({user: userId, room: roomId}),
];
setNextSyncData(eventData);
NEXT_SYNC_DATA.rooms.join[roomId].timeline.limited = true;

client.on("sync", function(state) {
if (state !== "PREPARED") {
return;
}
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(() => {
const room = client.getRoom(roomId);

let emitCount = 0;
Expand All @@ -590,14 +590,13 @@ describe("MatrixClient room timelines", function() {
});

httpBackend.flush("/messages", 1);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).done(function() {
return q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).then(function() {
expect(emitCount).toEqual(1);
done();
});
});
httpBackend.flush("/sync", 1);
});
});
});
Loading

0 comments on commit ae950a2

Please sign in to comment.