[COL-56] Add integration tests

[lawrence-forooghian]

This adds integration tests that test the core functionality of the SDK. It's currently a draft so I can get feedback on the approach and some of the questions that came up whilst I was working in this (see TODO comments in code).

On top of what's currently here, I intend to also add a test for component locking. (I've added a component locking test, too.)

[dpiatek]

The naming is good in this file, but I think some of the comments really get in the way of following efficiently what's happening in the file. I'd review every comment and focus on two things:

• is the intent of a given line of code clear or could be part of the summary?
• is an API problematic to test, we have to work around its limitations and explain why?

There are instances of both for sure in this file, but many comments, like the one above, feel very unnecessary or could be part of the utility function, instead of being repeated multiple times per test.

[lawrence-forooghian]

I've slimmed down the comments and will push the updated version shortly.

[dpiatek]

I really think we should try to take advantage of nested before and describe blocks instead of using comments for scenarios. It makes it easier to update and understand a test section-by-section instead of having to parse the whole test to debug it.

[lawrence-forooghian]

I'm afraid I'm not sure exactly what it is that you're suggesting here. What it sounds like is that, given a single test that’s written linearly like this:

scenario_1:
  doSomething_1
  checkSomething_1

scenario_2:
  doSomething_2
  checkSomething_2

scenario_3:
  doSomething_3
  checkSomething_3

then you're suggesting that we instead write it as nested test cases something like this?

describe('scenario_1', () => {
  beforeEach(() => {
    // do the actions of doSomething_1 and somehow expose a way for the `it` block to check its results
  });

  it('some description of scenario_1', () => {
    // do checkSomething_1
  });

  describe('scenario_2', () => {
    beforeEach(() => {
      // do the actions of doSomething_2 and somehow expose a way for the below `it` block to check its results
    });

    it('some description of scenario_2', () => {
      // do checkSomething_2
    });

    describe('scenario_3', () => {
      beforeEach(() => {
        // do the actions of doSomething_3 and somehow expose a way for the below `it` block to check its results
      });

      it('some description of scenario_3', () => {
        // do checkSomething_3
      });
    });
  });
});

Have I got that right? If so, a couple of things that I'd wonder about:

• isn't this code going to get very indented, very quickly?
• I'm not sure how it achieves "instead of having to parse the whole test to debug it" — given that each scenario relies on the side effects given in the preceding scenarios, don't you still have to understand all of the preceding sections in order to confidently edit a given section?

If I've misunderstood what you're suggesting, would you mind perhaps showing a concrete example of what you had in mind?

[dpiatek]

isn't this code going to get very indented, very quickly?

this is how the unit tests are structured as well, I'm not sure why that's an issue? (I might be biased - for me this is pretty standard in JavaScript and Ruby ecosystems).

I'm not sure how it achieves "instead of having to parse the whole test to debug it" — given that each scenario relies on the side effects given in the preceding scenarios, don't you still have to understand all of the preceding sections in order to confidently edit a given section?

with your current structure, if a test fails it's something like:

cursors test failed - failed
// expectation which failed

So we need to go over all the lines of code up to the point that a given expectation has failed. Compare that to:

cursors test
  assertion description and setup 1 - passed
  assertion description and setup 2 - passed
  assertion description and setup 3 - failed

Without looking at code or expectations, we can see what assertion, assumption fails, and also which ones are still good. With a linear setup, it's likely no assertion passes from the first failure, but that's fine. This is how Cucumber specs work in Ruby. https://github.com/ably/website/blob/main/features/authentication_a.feature#L11-L24

[lawrence-forooghian]

Just adding a comment to record that we had a chat about this and it turns out I was a bit thrown by your usage of the term "nested", where you actually just meant to keep the structure of the tests as-is but to nest each test case in a describe, put the setup part into a beforeAll and each scenario into its own it. I've made those changes and will push them shortly.

[dpiatek]

Why not use the listener for all events (i.e. not pass an event to members.subscribe) or use getAll to check the membership? These APIs will be much more commonly used than once in real-world apps.

[lawrence-forooghian]

Before I change this, I want to be sure I understand what you're suggesting. Are you saying that you don't want us to use once at all in this test file? And, if so, would using subscribe and then removing the subscriber when the listener is called — which would emulate the behaviour of once and keep the test structure as it is — be OK, or are you wanting us to only call subscribe once at the start of the test and then look at which events the listener receives as we perform various actions?

[lawrence-forooghian]

(and are you talking only about the members test, or all of them?)

[dpiatek]

no, this was just referring to members, and in particular the comment above and your worry about present events. once forces you to pass an event name, while you can use subscribe to capture everything, including present events (which honestly, as I write it, feels like a bug because it's not obvious at all).

And I proposed getAll because it waits for the presence set to sync, I thought would be useful in tests too (as at least in theory, you can use it to expect some state to sync - same as once). But I don't mind using once for tests - it's still a valid approach.

To be clear, I'm not requesting any changes here, I was responding more to the comment and reference to enter/present behaviour.

[dpiatek]

why use context.expect here and not expect if we importing descirbe and it ?

[lawrence-forooghian]

I was following the guidance for running tests in parallel:

When running concurrent tests, Snapshots and Assertions must use expect from the local Test Context to ensure the right test is detected.

[dpiatek]

gotcha 👍🏻

[lawrence-forooghian]

Well, now that we have nested describe blocks the tests are no longer working in parallel (will add a comment to commit message explaining what I tried), so I've switched everything back to using top-level expect anyway.

[dpiatek]

As an example of comments that I don't think are necessary (because the code reads almost as the English equivalent), I think all the comments from lines 86 up to here are not needed.

[dpiatek]

This is expected behaviour and your workaround is fine; if this isn't clear from docs we should update them.

[lawrence-forooghian]

I don't think it's clear from the docs. Is there any action needed on this PR related to this?

[lawrence-forooghian]

(I'll remove the TODO part of the comment)

[dpiatek]

Could the .channel property on cursors not be used for that?

[dpiatek]

We could also potentially expose a method for users to have more control of channel attachment, that's synced with what subscribe/unsubscribe does.

[lawrence-forooghian]

Yeah, I was originally hesitant about directly interacting with the underlying Ably channel in the tests, but I've changed my mind and will do so for this and for this one too.

[lawrence-forooghian]

We could also potentially expose a method for users to have more control of channel attachment, that's synced with what subscribe/unsubscribe does.

Yep, I've updated my comment to mention this as an option.

[dpiatek]

what happens if this timeout is removed? does the test become flaky?

[lawrence-forooghian]

Yes, performerSpace will drop the cursor updates and cursorUpdatesPromise will never resolve. I've added the following text to the comment to clarify this:

and hence not drop the cursor updates that we pass to set()

[dpiatek]

understood 👍🏻

[lawrence-forooghian]

As alluded to here I'm going to change this to make it more deterministic by observing performerSpace.cursors.channel.presence and removing the timeout.

[dpiatek]

This is a side-effect of how messages are processed right now; it is not desired but expected right now (there are tickets in the backlog to make that cleaner).

[lawrence-forooghian]

Is there an issue that I could add a link to here?

[dpiatek]

Not for this particular case, no - if you could open one, that would be great.

For now, I'd leave a comment here that this behaviour is expected, but not desired.

[lawrence-forooghian]

Created https://ably.atlassian.net/browse/COL-549 and will add a comment about it.

[lawrence-forooghian]

@dpiatek I've also added tests for locks.get() before and after acquiring a lock, will push shortly.

[dpiatek]

One final remark @lawrence-forooghian - do you think it's worth outlining the approach taken in this test at the top of the file, similar to what you have done in the commit? I think it might not be obvious that each it is treated as a step in the spec instead of an individual test, and they share state in a describe block.

[lawrence-forooghian]

Sure, have done.