Test improvements

[voliva]

Most of the work is already done, but there's still margin for improvement on the tests. I focused on those methods exposed as part of the API.

connectObservable

• It's missing a test for updates on the source stream.
• Q: I understand re-rxjs doesn't do any sort of batching. Is there any way to show how to make react batch updates?
• Maybe the test it works with suspense can be broken down. I haven't taken a deep look and I'm a bit out of the loop from suspense, but it will be interesting to see if we can break that test down.

connectFactoryObservable

• On test it returns the latest emitted value it only checks for the initial value. Needs to add in some updates on the source stream.
• I think it would be good to have a test it shares the subscription among all of the subscribers of the same key. It is superseeded by another test, but this will document a bit better how the key works.

    /**
     * - one hook subscribes to key1
     * - it receives the initial value
     * - perform a change on key1 subject
     * - another hook subscribes to key1
     * - both receive the change made on key1 subject
     * - another hook subscribes to key2, and receives the initial value
     * - all hooks unmount
     */

• The test observable > if the source observable completes it keeps emitting the latest value until there are no more subscriptions has an expect I found confusing:

  let diff = -1
  const [, getShared] = connectFactoryObservable((_: number) => {
    diff++
    return from([1, 2, 3, 4].map(val => val + diff))
  })

  let latestValue1: number = 0
  let nUpdates = 0
  const sub1 = getShared(0).subscribe(x => {
    latestValue1 = x
    nUpdates += 1
  })
  expect(latestValue1).toBe(4)
  expect(nUpdates).toBe(1) // <--- this one
  /**
   * Feels like it shouldn't be expected to receive only the last value from the source
   * as it seems like it's sort of debouncing the updates.
   * I've checked and it's not, it's only on this specific case where you have a synchronous
   * source that you will get the last value of that synchronous batch on the first subscription.
   * Following "synchronous batches" are fine (they will emit every single item), it's just
   * the initial one.
   * Maybe it's worth having a test just to have this case documented, even if the behaviour
   * changes later on if we consider this needs to be fixed.
   */

• The error handling ones we mentioned how having a test working with react would be cool. I also wonder how to replicate this test also for connectObservable (as it applies as well)

createInput

I found the test cover everything I could think of 👍

rxjs operators

• Maybe we also need a set of small tests for suspended and switchMapSuspended

[josepot]

Thanks a lot @voliva for looking into the tests and for coming up with ideas on how to improve them. The truth is that I'm not very happy with the tests that we got ATM. I think that the tests that we currently have can be decent for detecting when a refactor breaks something, but are horrible for documenting the behavior of the API...

I focused on those methods exposed as part of the API.
💯

connectObservable

• It's missing a test for updates on the source stream.

Totally, let's add those tests ASAP.

• Q: I understand re-rxjs doesn't do any sort of batching. Is there any way to show how to make react batch updates?

As we've discussed before, on Concurrent Mode observing external-inputs on an asapScheduler would help react to automatically batch those updates. On Legacy Mode, I'm pretty sure that we could accomplish the same thing if on top that we wrapped the observer.next calls with unstable_batchedupdates... We could make a tiny package that exported that operator... And this made me realize that we should probably tests this set of test with all the React versions that we want to support 🤔

• Maybe the test it works with suspense can be broken down. I haven't taken a deep look and I'm a bit out of the loop from suspense, but it will be interesting to see if we can break that test down.

I agree. ATM that test is a crappy way to test all the possible usages of the SUSPENSE operators.

connectFactoryObservable

• On test it returns the latest emitted value it only checks for the initial value. Needs to add in some updates on the source stream.

Agreed!

• I think it would be good to have a test it shares the subscription among all of the subscribers of the same key. It is superseeded by another test, but this will document a bit better how the key works.

Good point

    /**
     * - one hook subscribes to key1
     * - it receives the initial value
     * - perform a change on key1 subject
     * - another hook subscribes to key1
     * - both receive the change made on key1 subject
     * - another hook subscribes to key2, and receives the initial value
     * - all hooks unmount
     */

• The test observable > if the source observable completes it keeps emitting the latest value until there are no more subscriptions has an expect I found confusing:

  let diff = -1
  const [, getShared] = connectFactoryObservable((_: number) => {
    diff++
    return from([1, 2, 3, 4].map(val => val + diff))
  })

  let latestValue1: number = 0
  let nUpdates = 0
  const sub1 = getShared(0).subscribe(x => {
    latestValue1 = x
    nUpdates += 1
  })
  expect(latestValue1).toBe(4)
  expect(nUpdates).toBe(1) // <--- this one
  /**
   * Feels like it shouldn't be expected to receive only the last value from the source
   * as it seems like it's sort of debouncing the updates.
   * I've checked and it's not, it's only on this specific case where you have a synchronous
   * source that you will get the last value of that synchronous batch on the first subscription.

What the heck!? Maybe it's because I'm tired, but this really looks wrong to me and I don't quite understand why it's doing that... No, no, no, I think that this should be fixed... The distinctShareReplay shouldn't be behaving like that on its first subscription 🤔

• Following "synchronous batches" are fine (they will emit every single item), it's just
• the initial one.
• Maybe it's worth having a test just to have this case documented, even if the behaviour
• changes later on if we consider this needs to be fixed.
  */

* The error handling ones we mentioned how having a test working with react would be cool. I also wonder how to replicate this test also for `connectObservable` (as it applies as well)

Yep, the error handling ones should be improved. I agree.

createInput

I found the test cover everything I could think of

rxjs operators

Maybe we also need a set of small tests for suspended and switchMapSuspended

The problem is that AFAIK it's not possible to test them using marble-tests because the operators all emit this special value and every time I try to make marble-tests then the tests break because they don't recognize that value... But yeah, we should.

Thanks for the review @voliva ! Let's do this!