Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[COL-335] Fix bug where space.enter() sometimes hangs. #227

Merged
merged 7 commits into from
Oct 19, 2023
Merged

[COL-335] Fix bug where space.enter() sometimes hangs. #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
72696f8
Remove mocking of presence enter completion in space.enter() test
lawrence-forooghian Oct 19, 2023
f827d5a
Format auto-mocks
lawrence-forooghian Oct 18, 2023
c66bd09
Make each test context create new mock objects
lawrence-forooghian Oct 18, 2023
ce44eb9
Remove unnecessary space.enter() from some tests
lawrence-forooghian Oct 19, 2023
1431c3c
Remove usage of private presence API
lawrence-forooghian Oct 17, 2023
6f513a1
Check sender of ENTER presence message in space.enter()
lawrence-forooghian Oct 18, 2023
803416d
Fix bug where space.enter() sometimes wouldn’t return
lawrence-forooghian Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 50 additions & 34 deletions __mocks__/ably/promises/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,46 +5,59 @@ const MOCK_CLIENT_ID = 'MOCK_CLIENT_ID';
const mockPromisify = <T>(expectedReturnValue): Promise<T> => new Promise((resolve) => resolve(expectedReturnValue));
const methodReturningVoidPromise = () => mockPromisify<void>((() => {})());

const mockPresence = {
get: () => mockPromisify<Types.PresenceMessage[]>([]),
update: () => mockPromisify<void>(undefined),
enter: methodReturningVoidPromise,
leave: methodReturningVoidPromise,
subscriptions: {
once: (_: unknown, fn: Function) => {
fn();
function createMockPresence() {
return {
get: () => mockPromisify<Types.PresenceMessage[]>([]),
update: () => mockPromisify<void>(undefined),
enter: methodReturningVoidPromise,
leave: methodReturningVoidPromise,
subscriptions: {
once: (_: unknown, fn: Function) => {
fn();
},
},
},
subscribe: () => {},
};
subscribe: () => {},
unsubscribe: () => {},
};
}

const mockHistory = {
items: [],
first: () => mockPromisify(mockHistory),
next: () => mockPromisify(mockHistory),
current: () => mockPromisify(mockHistory),
hasNext: () => false,
isLast: () => true,
};
function createMockHistory() {
const mockHistory = {
items: [],
first: () => mockPromisify(mockHistory),
next: () => mockPromisify(mockHistory),
current: () => mockPromisify(mockHistory),
hasNext: () => false,
isLast: () => true,
};
return mockHistory;
}

const mockEmitter = {
any: [],
events: {},
anyOnce: [],
eventsOnce: {},
};
function createMockEmitter() {
return {
any: [],
events: {},
anyOnce: [],
eventsOnce: {},
};
}

const mockChannel = {
presence: mockPresence,
history: () => mockHistory,
subscribe: () => {},
publish: () => {},
subscriptions: mockEmitter,
};
function createMockChannel() {
return {
presence: createMockPresence(),
history: (() => {
const mockHistory = createMockHistory();
return () => mockHistory;
})(),
subscribe: () => {},
publish: () => {},
subscriptions: createMockEmitter(),
};
}

class MockRealtime {
public channels: {
get: () => typeof mockChannel;
get: () => ReturnType<typeof createMockChannel>;
};
public auth: {
clientId: string;
Expand All @@ -58,7 +71,10 @@ class MockRealtime {

constructor() {
this.channels = {
get: () => mockChannel,
get: (() => {
const mockChannel = createMockChannel();
return () => mockChannel;
})(),
};
this.auth = {
clientId: MOCK_CLIENT_ID,
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
"scripts": {
"lint": "eslint .",
"lint:fix": "eslint --fix .",
"format": "prettier --write --ignore-path .gitignore src demo",
"format:check": "prettier --check --ignore-path .gitignore src demo",
"format": "prettier --write --ignore-path .gitignore src demo __mocks__",
"format:check": "prettier --check --ignore-path .gitignore src demo __mocks__",
"test": "vitest run",
"test:watch": "vitest watch",
"coverage": "vitest run --coverage",
Expand Down
1 change: 0 additions & 1 deletion src/Locations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ describe('Locations', () => {

it<SpaceTestContext>('sends a presence update on location set', async ({ space, presence }) => {
const spy = vi.spyOn(presence, 'update');
await space.enter();
await space.locations.set('location1');
expect(spy).toHaveBeenCalledWith(createLocationUpdate({ current: 'location1' }));
});
Expand Down
12 changes: 0 additions & 12 deletions src/Locks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ describe('Locks', () => {
space,
presence,
}) => {
await space.enter();

const presenceUpdate = vi.spyOn(presence, 'update');

const lockID = 'test';
Expand All @@ -71,8 +69,6 @@ describe('Locks', () => {
});

it<SpaceTestContext>('includes attributes in the lock request when provided', async ({ space, presence }) => {
await space.enter();

const presenceUpdate = vi.spyOn(presence, 'update');

const lockID = 'test';
Expand All @@ -89,8 +85,6 @@ describe('Locks', () => {
});

it<SpaceTestContext>('errors if a PENDING request already exists', async ({ space }) => {
await space.enter();

const lockID = 'test';
await space.locks.acquire(lockID);
expect(space.locks.acquire(lockID)).rejects.toThrowError();
Expand All @@ -108,7 +102,6 @@ describe('Locks', () => {
});

it<SpaceTestContext>('sets a PENDING request to LOCKED', async ({ space }) => {
await space.enter();
const member = (await space.members.getSelf())!;

const emitSpy = vi.spyOn(space.locks, 'emit');
Expand Down Expand Up @@ -173,8 +166,6 @@ describe('Locks', () => {
},
])('$name', ({ desc, otherConnId, otherTimestamp, expectedSelfStatus, expectedOtherStatus }) => {
it<SpaceTestContext>(desc, async ({ client, space }) => {
await space.enter();

// process a PENDING request for the other member, which should
// transition to LOCKED
let msg = Realtime.PresenceMessage.fromValues({
Expand Down Expand Up @@ -230,7 +221,6 @@ describe('Locks', () => {
});

it<SpaceTestContext>('sets a released request to UNLOCKED', async ({ space }) => {
await space.enter();
const member = (await space.members.getSelf())!;

let msg = Realtime.PresenceMessage.fromValues({
Expand Down Expand Up @@ -263,7 +253,6 @@ describe('Locks', () => {
});

it<SpaceTestContext>('sets all locks to UNLOCKED when a member leaves', async ({ space }) => {
await space.enter();
const member = (await space.members.getSelf())!;

let msg = Realtime.PresenceMessage.fromValues({
Expand Down Expand Up @@ -311,7 +300,6 @@ describe('Locks', () => {
});

it<SpaceTestContext>('removes the identified lock request from presence extras', async ({ space, presence }) => {
await space.enter();
const member = (await space.members.getSelf())!;

const lockID = 'test';
Expand Down
53 changes: 51 additions & 2 deletions src/Space.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,59 @@ describe('Space', () => {
});

describe('enter', () => {
beforeEach<SpaceTestContext>(({ presence }) => {
vi.spyOn(presence, 'subscribe').mockImplementation(
async (_, listener?: (presenceMessage: Types.PresenceMessage) => void) => {
listener!(
createPresenceMessage('enter' /* arbitrarily chosen */, { clientId: 'MOCK_CLIENT_ID', connectionId: '1' }),
);
},
);
});

it<SpaceTestContext>('enter a space successfully', async ({ space, presence }) => {
const spy = vi.spyOn(presence, 'enter').mockResolvedValueOnce();
const presenceEnterSpy = vi.spyOn(presence, 'enter');
const presenceSubscribeSpy = vi.spyOn(presence, 'subscribe');
await space.enter({ name: 'Betty' });
expect(spy).toHaveBeenNthCalledWith(1, createProfileUpdate({ current: { name: 'Betty' } }));
expect(presenceEnterSpy).toHaveBeenNthCalledWith(1, createProfileUpdate({ current: { name: 'Betty' } }));
expect(presenceSubscribeSpy).toHaveBeenCalledWith(['enter', 'present'], expect.any(Function));
});

describe.each([
{
scenario: 'when it receives a presence message from a client ID and connection ID that matches ours',
presenceMessageData: { clientId: 'MOCK_CLIENT_ID', connectionId: '1' },
shouldComplete: true,
},
{
scenario: 'when it receives a presence message from a client ID that isn’t ours',
presenceMessageData: { clientId: 'OTHER_MOCK_CLIENT_ID', connectionId: '1' },
shouldComplete: false,
},
{
scenario: 'when it receives a presence message from a connection ID that isn’t ours',
presenceMessageData: { clientId: 'MOCK_CLIENT_ID', connectionId: '2' },
shouldComplete: false,
},
])('$scenario', ({ presenceMessageData, shouldComplete }) => {
it<SpaceTestContext>(shouldComplete ? 'completes' : 'does not complete', async ({ space, presence }) => {
const unsubscribeSpy = vi.spyOn(presence, 'unsubscribe');

vi.spyOn(presence, 'subscribe').mockImplementation(
async (_, listener?: (presenceMessage: Types.PresenceMessage) => void) => {
listener!(createPresenceMessage('enter' /* arbitrarily chosen */, presenceMessageData));
},
);

space.enter();

// Note: There’s no nice way (i.e. without timeouts) to actually check that space.enter() didn’t complete, so we use "did it remove its presence listener?" as a proxy for "did it complete?"
if (shouldComplete) {
expect(unsubscribeSpy).toHaveBeenCalled();
} else {
expect(unsubscribeSpy).not.toHaveBeenCalled();
}
});
});

it<SpaceTestContext>('returns current space members', async ({ presenceMap, space }) => {
Expand Down
19 changes: 14 additions & 5 deletions src/Space.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,18 +244,27 @@ class Space extends EventEmitter<SpaceEventMap> {
return new Promise((resolve) => {
const presence = this.channel.presence;

interface PresenceWithSubscriptions extends Types.RealtimePresencePromise {
subscriptions: EventEmitter<{ enter: [unknown] }>;
}
const presenceListener = async (presenceMessage: Types.PresenceMessage) => {
if (
!(
presenceMessage.clientId == this.client.auth.clientId &&
presenceMessage.connectionId == this.client.connection.id
)
) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add test coverage for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be good to test it, but I don't have any good ideas on how. We want to test that space.enter() doesn’t return as a result of receiving a presence message whose client ID or connection ID doesn't match ours, right? I'm not sure of a good way of testing "doesn’t return" without introducing a timeout, something like this:

    it<SpaceTestContext>('doesn’t complete as a result of a presence message from a client ID that is not ours', async ({
      space,
      presence,
    }) => {
      let hasEmittedPresenceMessageWithCorrectClientId = false;
      vi.spyOn(presence, 'subscribe').mockImplementation(
        async (_, listener?: (presenceMessage: Types.PresenceMessage) => void) => {
          listener!(
            createPresenceMessage('enter', {
              clientId: 'OTHER_MOCK_CLIENT_ID',
              connectionId: '1',
            }),
          );
          setTimeout(() => {
            hasEmittedPresenceMessageWithCorrectClientId = true;
            listener!(
              createPresenceMessage('enter', {
                clientId: 'MOCK_CLIENT_ID',
                connectionId: '1',
              }),
            );
          }, 500);
        },
      );

      await space.enter();
      expect(hasEmittedPresenceMessageWithCorrectClientId).to.be.true;
    });

How do you feel about that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you spyOn on subscribe that it gets called twice but unsubscribe just once & expect that the promise only resolves after the second message, not the first? Do you need a timeout here, or could maybe just call them one after the other?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you spyOn on subscribe that it gets called twice but unsubscribe just once

That's not the expected behaviour — space.enter() only ever calls subscribe once, but it doesn't call unsubscribe until it receives a matching presence message.

Do you need a timeout here, or could maybe just call them one after the other?

I'm not sure exactly what you're suggesting, do you mean something like this?

    it<SpaceTestContext>('doesn’t complete as a result of a presence message from a client ID that is not ours', async ({
      space,
      presence,
    }) => {
      let hasEmittedPresenceMessageWithCorrectClientId = false;
      vi.spyOn(presence, 'subscribe').mockImplementation(
        async (_, listener?: (presenceMessage: Types.PresenceMessage) => void) => {
          listener!(
            createPresenceMessage('enter', {
              clientId: 'OTHER_MOCK_CLIENT_ID',
              connectionId: '1',
            }),
          );
          hasEmittedPresenceMessageWithCorrectClientId = true;
          listener!(
            createPresenceMessage('enter', {
              clientId: 'MOCK_CLIENT_ID',
              connectionId: '1',
            }),
          );
        },
      );

      await space.enter();
      expect(hasEmittedPresenceMessageWithCorrectClientId).to.be.true;
    });

If so, that test wouldn't really be testing anything — it would continue to pass even if you removed the client ID check from the implementation of space.enter().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, had a stab at it - bit different then I described, but how about:

it<SpaceTestContext>('doesn’t complete as a result of a presence message from a client ID that is not ours', async ({
  space, 
  presence,
}) => {     
  const unsubscribeSpy = vi.spyOn(presence, 'unsubscribe');

  vi.spyOn(presence, 'subscribe').mockImplementation(
    async (_, listener?: (presenceMessage: Types.PresenceMessage) => void) => {
      listener!(
        createPresenceMessage('enter', {
          clientId: 'OTHER_MOCK_CLIENT_ID', // a clientId that does not match our setup mock
          connectionId: '1',
        }),
      );
    },
  );

  Promise.resolve(space.enter())
  expect(unsubscribeSpy).not.toHaveBeenCalled()
}, 1000);


it<SpaceTestContext>('completes as a result of a presence message from a client ID that is ours', async ({
  space,
  presence,
}) => {
  const unsubscribeSpy = vi.spyOn(presence, 'unsubscribe');

  vi.spyOn(presence, 'subscribe').mockImplementation(
    async (_, listener?: (presenceMessage: Types.PresenceMessage) => void) => {
      listener!(
        createPresenceMessage('enter', {
          clientId: 'MOCK_CLIENT_ID', // a clientId that matches our setup mock
          connectionId: '1',
        }),
      );
    },
  );

  Promise.resolve(space.enter())
  expect(unsubscribeSpy).toHaveBeenCalled()
});

Copy link
Contributor

@dpiatek dpiatek Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the timeout should be removed, was experimenting with how we can test enter not resolving.

Promise.resolve is a bit of trick - It doesn't do anything except pass the promise returned from enter, but it allows us to call the function without an await, which would cause the test to wait. This could also be accomplished with an IIFE, albeit it would be more verbose.

to check whether presence.unsubscribe has been called instead of checking whether space.enter() has returned?

yes, that's the key idea - still not ideal, but avoids any complexity arising from timers, for example (and is fairly concise).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it allows us to call the function without an await

Can't we just do that anyway? An async function is just really a function that returns a Promise, I thought. Why would we need to await its return value in order to be able to call it?

yes, that's the key idea

Cool, I'll write something based around this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just do that anyway? An async function is just really a function that returns a Promise, I thought. Why would we need to await its return value in order to be able to call it?

you can, but then you need to add an exception for typescript (or eslint? can't remember). I don't really mind what we do here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can, but then you need to add an exception for typescript (or eslint? can't remember)

Ah, OK — well, I've removed the Promise.resolve and nothing seems to be complaining, so I'll keep it like that 😄

Copy link
Collaborator Author

@lawrence-forooghian lawrence-forooghian Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've updated the tests in a4e0351, please could you take a look?


presence.unsubscribe(presenceListener);

(presence as PresenceWithSubscriptions).subscriptions.once('enter', async () => {
const presenceMessages = await presence.get();

presenceMessages.forEach((msg) => this.locks.processPresenceMessage(msg));

const members = await this.members.getAll();
resolve(members);
});
};

presence.subscribe(['enter', 'present'], presenceListener);

const update = new SpaceUpdate({ self: null, extras: null });
this.presenceEnter(update.updateProfileData(profileData));
Expand Down
Loading