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

[lawrence-forooghian]

This fixes a bug where space.enter() would sometimes hang. See commit messages for more details.

[dpiatek]

I'd propose we do this a bit differently. I think having mockSubscribeForSpaceEnter in different tests will just make it harder to decide if that method should be used or the alternative presenceMap approach, which sets the presence set, without needing to call enter().

So my suggestion would be to for this spec (that tests enter) use a beforeEach that sets up the mock for presence, but all the other specs are refactored to not call enter() but just use the presenceMap approach. WDYT?

[lawrence-forooghian]

It sounds reasonable. I'll have to take a bit of time to understand how the presenceMap is currently used, which I'll do now.

[lawrence-forooghian]

Turns out that none of these space.enter() calls in the other tests were needed, so I've removed them.

[lawrence-forooghian]

(see ce44eb9)

[lawrence-forooghian]

I've moved the mocking of presence.subscribe into the beforeEach of enter’s tests, as you suggested.

[dpiatek]

Let's add test coverage for this

[lawrence-forooghian]

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?

[dpiatek]

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?

[lawrence-forooghian]

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().

[dpiatek]

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()
});

[dpiatek]

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).

[lawrence-forooghian]

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.

[dpiatek]

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.

[lawrence-forooghian]

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 😄

[lawrence-forooghian]

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

[lawrence-forooghian]

@dpiatek I've also updated the test for space.enter() to check that it subscribes to both enter and present events.