COL-685: Write Models integration tests

[snikidev]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
models	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 14, 2024 0:14am

[mschristensen]

Great first pass, left some comments :)

[mschristensen]

We should also await subscriptionCalls[0] on L119 and assert initial state, then subscriptionCalls[1] on L132 and assert optimistic state, and finally subscriptionCalls[2] and assert confirmed state

[snikidev]

when you say "initial state" do you mean model.state? Because model.data only has optimistic and confirmed doesn't it?

[mschristensen]

I mean the initial value that the model state should hold after the sync call, i.e. the initial value of model.data.confirmed before the published message is processed

[mschristensen]

we're also currently asserting only on the value of model.data.confirmed/optimistic rather than the value the the subscription callback is actually invoked with, that would be worth asserting on too

[mschristensen]

We should still await subscriptionCalls[0] and assert initial state, and assert that the confirmation promise returned by the optimistic call rejects.

[snikidev]

have a look, did I do it as you meant it?

[mschristensen]

This test doesn't assert that confirmation rejects, tests will pass if it resolves as the catch block is never run

[mschristensen]

You can use await expect(promise).rejects.toThrow()

[mschristensen]

This test should really be the same as the one above, i.e. it still asserts that the subscribe callback is invoked correctly

[mschristensen]

Not to be a party pooper, but this naming is probably confusing; there's nothing special about this even that makes it fast, so I'd prefer to just call it 'event' or something sensible 😛

[mschristensen]

Here we are setting the optimistic data to the same data the model was intialised with from the sync call. I think it would be more robust to apply different data optimistically in this test (and in the others) and assert on that

[mschristensen]

This might be a confusing property, as 34 messages are never sent; would be better to just call the key foo

[mschristensen]

I'm not sure this is doing what we expect:

1. The initial confirmed data is eventData.data as that is what is passed to sync
2. We then apply the same event optimistically (no detectable change in the data that we can assert on)
3. We then publish the confirmed update blazinglyFastEvent; eventData.data will be rebased
4. We then confirm eventData.data

We should fix 2. (this issue is present in other tests per prev comment)

We should also avoid using the spread operator here to merge in object keys, as this doesn't allow us to tell the order in which updates were applied (since we are using distinct keys so both will be present in the resulting object regardless of the order the updates are applied in). Instead it would be better to use e.g. an array, which allows us to see the order, i.e. that blazinglyFastEvent comes first and eventData comes second.

[snikidev]

I was lazy and didn't change the sync data into an array but I did change the object properties, so it's more visible as to what data came in first and last. Let me know if you don't like it, I'll change that into an array. 🙃

[mschristensen]

Another thought: it would be good to add a test of the replay from history functionality too: https://ably.com/docs/livesync/models/models#history

[mschristensen]

last missing piece: let's capture the confirmation promise here:

const [confirmation] = await model.optimistic(eventData);

And then later, assert it rejects:

await expect(confirmation).rejects.toThrow()

[mschristensen]

Approving this as-is and leaving final comment on confirmation promise bug to be addressed under DTP-720