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 c513a82
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 73 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
50 changes: 29 additions & 21 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 @@ -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() {
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 Down Expand Up @@ -440,9 +442,10 @@ describe("MatrixClient room timelines", function() {
return;
}
const room = client.getRoom(roomId);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).then(function() {
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);
Expand Down Expand Up @@ -471,9 +474,10 @@ describe("MatrixClient room timelines", function() {
nameEmitCount += 1;
});

httpBackend.flush("/sync", 1).then(() => {
return utils.syncPromise(client);
}).done(function() {
q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).done(function() {
expect(nameEmitCount).toEqual(1);
expect(room.name).toEqual("Room 2");
// do another round
Expand All @@ -485,9 +489,10 @@ 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() {
q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).done(function() {
expect(nameEmitCount).toEqual(2);
expect(room.name).toEqual("Room 3");
done();
Expand Down Expand Up @@ -517,9 +522,10 @@ describe("MatrixClient room timelines", function() {
return;
}
const room = client.getRoom(roomId);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).then(function() {
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 Down Expand Up @@ -550,9 +556,10 @@ describe("MatrixClient room timelines", function() {
const room = client.getRoom(roomId);

httpBackend.flush("/messages", 1);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).done(function() {
q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).done(function() {
expect(room.timeline.length).toEqual(1);
expect(room.timeline[0].event).toEqual(eventData[0]);
expect(room.currentState.getMembers().length).toEqual(2);
Expand Down Expand Up @@ -590,9 +597,10 @@ describe("MatrixClient room timelines", function() {
});

httpBackend.flush("/messages", 1);
httpBackend.flush("/sync", 1).then(function() {
return utils.syncPromise(client);
}).done(function() {
q.all([
httpBackend.flush("/sync", 1),
utils.syncPromise(client),
]).done(function() {
expect(emitCount).toEqual(1);
done();
});
Expand Down
60 changes: 20 additions & 40 deletions spec/integ/megolm-integ.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,7 @@ describe("megolm", function() {
};

aliceTestClient.httpBackend.when("GET", "/sync").respond(200, syncResponse);
return aliceTestClient.httpBackend.flush("/sync", 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
const room = aliceTestClient.client.getRoom(ROOM_ID);
const event = room.getLiveTimeline().getEvents()[0];
Expand Down Expand Up @@ -387,9 +385,7 @@ describe("megolm", function() {
};

aliceTestClient.httpBackend.when("GET", "/sync").respond(200, syncResponse);
return aliceTestClient.httpBackend.flush("/sync", 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
const room = aliceTestClient.client.getRoom(ROOM_ID);
const event = room.getLiveTimeline().getEvents()[0];
Expand All @@ -404,9 +400,7 @@ describe("megolm", function() {
};

aliceTestClient.httpBackend.when("GET", "/sync").respond(200, syncResponse);
return aliceTestClient.httpBackend.flush("/sync", 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
const room = aliceTestClient.client.getRoom(ROOM_ID);
const event = room.getLiveTimeline().getEvents()[0];
Expand Down Expand Up @@ -474,9 +468,10 @@ describe("megolm", function() {
};
aliceTestClient.httpBackend.when("GET", "/sync").respond(200, syncResponse2);

return aliceTestClient.httpBackend.flush("/sync", 2);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
// flush both syncs
return aliceTestClient.flushSync().then(() => {
return aliceTestClient.flushSync();
});
}).then(function() {
const room = aliceTestClient.client.getRoom(ROOM_ID);
const event = room.getLiveTimeline().getEvents()[0];
Expand Down Expand Up @@ -504,9 +499,7 @@ describe("megolm", function() {
syncResponse.to_device = { events: [olmEvent] };

aliceTestClient.httpBackend.when('GET', '/sync').respond(200, syncResponse);
return aliceTestClient.httpBackend.flush('/sync', 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
// start out with the device unknown - the send should be rejected.
aliceTestClient.httpBackend.when('POST', '/keys/query').respond(
Expand Down Expand Up @@ -571,9 +564,7 @@ describe("megolm", function() {
const syncResponse = getSyncResponse(['@bob:xyz']);
aliceTestClient.httpBackend.when('GET', '/sync').respond(200, syncResponse);

return aliceTestClient.httpBackend.flush('/sync', 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
console.log("Forcing alice to download our device keys");

Expand Down Expand Up @@ -620,9 +611,7 @@ describe("megolm", function() {
syncResponse.to_device = { events: [olmEvent] };
aliceTestClient.httpBackend.when('GET', '/sync').respond(200, syncResponse);

return aliceTestClient.httpBackend.flush('/sync', 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
console.log('Forcing alice to download our device keys');

Expand Down Expand Up @@ -673,9 +662,7 @@ describe("megolm", function() {
syncResponse.to_device = { events: [olmEvent] };
aliceTestClient.httpBackend.when('GET', '/sync').respond(200, syncResponse);

return aliceTestClient.httpBackend.flush('/sync', 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
console.log("Fetching bob's devices and marking known");

Expand Down Expand Up @@ -904,9 +891,7 @@ describe("megolm", function() {
syncResponse.to_device = { events: [olmEvent] };

aliceTestClient.httpBackend.when('GET', '/sync').respond(200, syncResponse);
return aliceTestClient.httpBackend.flush('/sync', 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
console.log('Forcing alice to download our device keys');

Expand Down Expand Up @@ -938,9 +923,7 @@ describe("megolm", function() {
return aliceTestClient.start().then(() => {
aliceTestClient.httpBackend.when('GET', '/sync').respond(
200, getSyncResponse(['@bob:xyz', '@chris:abc']));
return aliceTestClient.httpBackend.flush('/sync', 1);
}).then(() => {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(() => {
// to make sure the initial device queries are flushed out, we
// attempt to send a message.
Expand Down Expand Up @@ -979,9 +962,10 @@ describe("megolm", function() {
changed: ['@chris:abc'],
},
});
return aliceTestClient.httpBackend.flush('/sync', 2);
}).then(() => {
return testUtils.syncPromise(aliceTestClient.client);
// flush both syncs
return aliceTestClient.flushSync().then(() => {
return aliceTestClient.flushSync();
});
}).then(() => {
// check that we don't yet have a request for chris's devices.
aliceTestClient.httpBackend.when('POST', '/keys/query', {
Expand All @@ -1006,7 +990,7 @@ describe("megolm", function() {
.getEndToEndDeviceTrackingStatus()['@chris:abc'];
if (chrisStat != 1 && chrisStat != 2) {
throw new Error('Unexpected status for chris: wanted 1 or 2, got ' +
bobStat);
chrisStat);
}

// now add an expectation for a query for bob's devices, and let
Expand Down Expand Up @@ -1098,9 +1082,7 @@ describe("megolm", function() {
};

aliceTestClient.httpBackend.when("GET", "/sync").respond(200, syncResponse);
return aliceTestClient.httpBackend.flush("/sync", 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
const room = aliceTestClient.client.getRoom(ROOM_ID);
const event = room.getLiveTimeline().getEvents()[0];
Expand Down Expand Up @@ -1132,9 +1114,7 @@ describe("megolm", function() {
};

aliceTestClient.httpBackend.when("GET", "/sync").respond(200, syncResponse);
return aliceTestClient.httpBackend.flush("/sync", 1);
}).then(function() {
return testUtils.syncPromise(aliceTestClient.client);
return aliceTestClient.flushSync();
}).then(function() {
const room = aliceTestClient.client.getRoom(ROOM_ID);
const event = room.getLiveTimeline().getEvents()[0];
Expand Down

0 comments on commit c513a82

Please sign in to comment.