Azure-sdk-for-js: [Service Bus] Streaming Tests fail often when peek() finds messages but expects none

[ramya-rao-a]

A lot of tests in the streamingReceiver.spec.ts file are failing often with the error
Unexpected number of msgs found when peeking: expected 1 to equal 0

This error comes from the peek check we make via testPeekMsgsLength

Here is the sequence of events in such test cases:

• Streaming receiver is run. Either the msg is completed or deadlettered in the onMessage handler or autoComplete is enabled
• checkWithTimeout is used to wait until the expected number of messages are received
• We make the checks on the messages received
• We use testPeekMsgsLength to ensure that the queue/subscription is empty

Now, the process of settling a message is asynchronous and the time taken by it depends on the network.

Since in step 2 above, we only wait until the messages are received and not until the message is settled, we can often get to step 4 before the message gets settled.

Proposed solution:

• When settling message in the onMessage handler, push the message to the array after settling the message. This way, checkWithTimeout will wait until the message is settled
• Avoid depending on the autoComplete feature to clean up the queue/subscription

@HarshaNalluru @ramya0820 Thoughts?

[HarshaNalluru]

@ramya-rao-a
This makes so much sense in the case of occasional failures in Streaming receiver tests.
So, the issue is not with peek, but with the message settling before peek.
If this thing fixes the tests, we would be able to check the servicebus-entity using testPeekMsgsLength at the end of all the tests. [Which was something I wanted to add in the should.equal-error PR]
Cheers!

[HarshaNalluru]

Avoid depending on the autoComplete feature to clean up the queue/subscription

Instead of avoiding autoComplete, can we check some property on message, which says whether the message is settled ?
If we do this, we can push the message only if message_settled = true [and we don't have to make too many changes to the tests].

[ramya-rao-a]

Good idea!

So you are saying, instead of using say receivedMsgs.length === 1 as a check we can do receivedMsgs.length === 1 && receivedMsgs[0].delivery.remote_settled === true?

[HarshaNalluru]

Yes. That also works. But we would have to handle the case where we send multiple messages in a different manner.

I suggest, we push the message in receivedMsgs in OnMessage after waiting for this(delivery.remote_settled === true) to happen.
That way, we don't have to handle multiple messages scenario.

      const settled = await checkWithTimeout(() => msg.delivery.remote_settled === true);
      if (settled) receivedMsgs.push(msg);

This way we don't need to make too many changes.
Is this way fine? Any thoughts ?

[ramya-rao-a]

In case of autoComplete, the message gets auto completed after the onMessage handler is done executing. In your case we will end up in an infinite loop. onMessage will not exit unless message is settled. Message is not auto completed until onMessage exits

[HarshaNalluru]

receivedMsgs.length === 1 && receivedMsgs[0].delivery.remote_settled === true should be good then.
(And a similar way for multiple messages. checking delivery.remote_settled === true for each message along with the expected_number_of_messages)

[ramya-rao-a]

Done with #1312