Update failing tests in EventHubs (misc.spec.ts receiver.spec.ts) #3709

ramya0820 · 2019-06-12T18:25:53Z

For more context refer to #1751

ramya-rao-a

Please see #1751 (comment) as to why skipping the initial receive call will not avoid the case of pending credits

ramya0820 · 2019-06-12T20:59:51Z

Please see #1751 (comment) as to why skipping the initial receive call will not avoid the case of pending credits

Please see updated PR.

My concern was that the tests were doing too many things (additional before and after state checks on eventhub) and could do with testing just the particular usecase (with one send and receive, in which case there wouldn't be any need to deal with pending credits being misused during the test runs).
Since it seems like we still want to have those in, updated tests with few corrections that I think were needed as well.

ramya0820 · 2019-06-12T21:09:13Z

Also, these changes would fix root cause for #3471 and #3472 as well which together are same as ones listed in #1751 but for browser.
Those tests identified the same issues issues consistently in browser. But in case of Node, the event loop was masking these sometimes due to likely a certain discrepancy with use of Node event loop framework that needs more investigation (as we have seen different behavior based on which line we add certain code involving network calls and timers).

ramya-rao-a · 2019-06-13T03:20:31Z

sdk/eventhub/event-hubs/test/receiver.spec.ts

      await client.send(ed, partitionId);
      debug(">>>>>>> Sent the new message after creating the receiver. We should only receive this message.");
-      const data2 = await breceiver.receive(10, 20);
+      const data2 = await client.receiveBatch(partitionId, 10, 20, {


data2 -> data
Same in a few other places

Got it, I was wondering if we should do one round of sole clean / improve, refactor of all eventhub tests PR and keep changes to it? That way we can get past the functional issues for now

ramya-rao-a · 2019-06-13T03:25:42Z

sdk/eventhub/event-hubs/test/receiver.spec.ts

@@ -99,27 +94,24 @@ describe("EventHub Receiver", function(): void {
          stamp: uid
        }
      };
+
+      const offset = (await client.getPartitionInformation(partitionId)).lastEnqueuedOffset;


This particular test is for from end of stream i.e testing of the event position as returned by EventPosition.fromEnd(). We have now updated it to use the lastEnqueuedOffset instead which makes this test identical to the one for after a particular offset

I don't see a way out for this case. Let's comment this test out and add a TODO comment to bring it back as part of #3714

Oh, there might have been a mistake, thanks for the catch!
This should be updated to EventPosition.fromEnd() and expect to receive 0 messages.

The intent was not to check that receiving from "end of stream" receives 0 messages. The intent was that a receiver created using the EventPosition.fromEnd(), will receive the messages that were sent after creating the receiver.

This is not possible without having the initial receive() call because the link is established only when the receive() call is made.

Therefore, my suggestion in #3709 (comment) was to undo the changes to this test and comment it out

The intent was not to check that receiving from "end of stream" receives 0 messages. The intent was that a receiver created using the EventPosition.fromEnd(), will receive the messages that were sent after creating the receiver.

This is not possible without having the initial receive() call because the link is established only when the receive() call is made.

Therefore, my suggestion in #3709 (comment) was to undo the changes to this test and comment it out

Oh, okay - will undo and comment out.

Although, I think the basic, happy case test for endOfStream would be that no messages are received when attempted to receive from end of stream. So the first check where inspite of sending (ten) messages to it, we don't receive any seems sufficient. The extra send receive after endOfStream seems extraneous to me, but maybe there's something more going on behind the creation of receiver that's being tested..

Agreed with the basic happy path, but since the intent of the test was more than just the happy path, I would prefer to comment it out.

The main intention of this exercise/PR/issue is to fix the flakiness. As part of the Track 2 work, we will look into increasing test coverage where we will revisit tests and maybe add this happy path.

ramya-rao-a · 2019-06-13T20:45:42Z

sdk/eventhub/event-hubs/test/receiver.spec.ts

@@ -197,18 +194,15 @@ describe("EventHub Receiver", function(): void {
      };
      await client.send(ed, partitionId);
      debug("Sent the new message after creating the receiver. We should only receive this message.");
-      const data = await breceiver.receive(10, 20);
+      const data = await client.receiveBatch(partitionId, 10, 20, {
+        eventPosition: EventPosition.fromOffset(pInfo.lastEnqueuedOffset)


We are changing what is being tested here.
This should be EventPosition.fromEnqueuedTime(pInfo.lastEnqueuedTimeUtc)

ramya-rao-a · 2019-06-13T20:48:28Z

sdk/eventhub/event-hubs/test/receiver.spec.ts

-    it("'from end of stream' should receive messages correctly ", async function(): Promise<void> {
+
+    // TODO: Enable following test as part of https://github.com/Azure/azure-sdk-for-js/issues/3714
+    // After the implementation of BatchingReceiver has been fixed


After the implementation of BatchingReceiver has been fixed
->
After we ensure the multiple receiveBatch calls on the user facing receiver work as expected

ramya0820 requested review from AlexGhiondea, mikeharder, HarshaNalluru and ramya-rao-a June 12, 2019 18:25

ramya0820 self-assigned this Jun 12, 2019

ramya-rao-a suggested changes Jun 12, 2019

View reviewed changes

Update tests

c4c97cd

ramya0820 force-pushed the issue-1751 branch from 6a54e65 to c4c97cd Compare June 12, 2019 20:55

ramya0820 mentioned this pull request Jun 12, 2019

[JS][Event Hubs] BatchingReceiver.receive() occasionally fails #1751

Closed

ramya0820 changed the title ~~Skip initial receive call~~ Update failing tests in EventHubs (misc.spec.ts receiver.spec.ts) Jun 12, 2019

ramya0820 requested a review from ShivangiReja June 12, 2019 22:00

Update tests

13cce01

ramya0820 force-pushed the issue-1751 branch from f8ea0fa to 13cce01 Compare June 13, 2019 01:10

Enable test for browser run

ae3999f

ramya-rao-a suggested changes Jun 13, 2019

View reviewed changes

Fix end of stream test

468e371

ramya0820 requested a review from ramya-rao-a June 13, 2019 18:11

ramya-rao-a mentioned this pull request Jun 13, 2019

[Event Hubs] Batching receiver with pending credits will result in missing events for user #3714

Closed

Disable test

610aa44

ramya-rao-a reviewed Jun 13, 2019

View reviewed changes

ramya0820 and others added 2 commits June 13, 2019 14:32

Address comments

0ddbc5e

Merge branch 'master' into issue-1751

9c10d6d

ramya-rao-a approved these changes Jun 14, 2019

View reviewed changes

ramya0820 added 3 commits June 14, 2019 10:43

Merge branch 'master' into issue-1751

f81a1df

Merge branch 'master' into issue-1751

7207381

Merge branch 'master' into issue-1751

bee9791

ramya0820 merged commit 972c84a into Azure:master Jun 15, 2019

ShivangiReja mentioned this pull request Jun 19, 2019

[Event Hubs] Close batching receiver link when returning the events to the user is less than maxMessageCount #3926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update failing tests in EventHubs (misc.spec.ts receiver.spec.ts) #3709

Update failing tests in EventHubs (misc.spec.ts receiver.spec.ts) #3709

ramya0820 commented Jun 12, 2019

ramya-rao-a left a comment

ramya0820 commented Jun 12, 2019 •

edited

Loading

ramya0820 commented Jun 12, 2019

ramya-rao-a Jun 13, 2019

ramya0820 Jun 13, 2019

ramya-rao-a Jun 13, 2019

ramya0820 Jun 13, 2019

ramya0820 Jun 13, 2019

ramya-rao-a Jun 13, 2019

ramya0820 Jun 13, 2019 •

edited

Loading

ramya-rao-a Jun 13, 2019

ramya-rao-a Jun 13, 2019

ramya-rao-a Jun 13, 2019

Update failing tests in EventHubs (misc.spec.ts receiver.spec.ts) #3709

Update failing tests in EventHubs (misc.spec.ts receiver.spec.ts) #3709

Conversation

ramya0820 commented Jun 12, 2019

ramya-rao-a left a comment

Choose a reason for hiding this comment

ramya0820 commented Jun 12, 2019 • edited Loading

ramya0820 commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya0820 Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya0820 commented Jun 12, 2019 •

edited

Loading

ramya0820 Jun 13, 2019 •

edited

Loading