[Event Hubs] Add tests for browser mode

[ramya-rao-a]

Just like what we are doing for Service Bus in #2163 , we need to do the same for Event Hubs

This task needs to be taken up after the #2808 is done

• Figure out what tests we want to run in Browser mode. Will they be same or subset of Nodejs? Will we need any new ones?
• Ensure we have a way to run the browser tests locally
• Ensure we can run the browser tests in CI

[ramya0820]

Regarding the test failures surfaced by #3445, following is a summary -

• One of them is to do with WebSocket throwing an error, though the node tests pass (iotHub.spec.ts)
• Some tests in misc.spec.ts are failing specific to batched messages, and only in browser and not for node. Needs further investigation.
• Some other tests in receiver.spec.ts are failing as being unable to read from stream seems to intermittently occur in Node as well. This may have to do with entity setup / clearing and requires further investigation.

If not resolved as part of #3445, will be tracking these issues separately.

cc @ramya-rao-a @AlexGhiondea @bterlson

[ramya0820]

Also, regarding what tests to run in browser, given recent findings and that there is no specific pattern in the browser test failures, all tests seem to be relevant in context of browsers. Hence the proposal would be to run all of them in browser.
@ramya-rao-a Thoughts?

Edit:

• packageInfo related test can be left specific to Node.
• And based on what we decide, the test tags can be manipulated accordingly such as by using grep or invert, whichever's easier/faster to maintain.

[ramya-rao-a]

Event Hubs has a much smaller set of tests when compared to Service Bus, so I am fine running all the tests in browser as well.

[ramya-rao-a]

Regarding the Websocket error in the test cases for IotHub, please log a new issue. It could very well be the case that IotHub service does not support Websockets. If that is indeed the case, then we need to update createFromIotHubConnectionString() to throw an error when we detect that Websockets are being used.

[ramya0820]

Regarding the Websocket error in the test cases for IotHub, please log a new issue. It could very well be the case that IotHub service does not support Websockets. If that is indeed the case, then we need to update createFromIotHubConnectionString() to throw an error when we detect that Websockets are being used.

Got it, logged #3473 for the iotHub testcases.
#3472 and #3471 have been logged as well for receiver.spec.ts and misc.spec.ts test failures.

[ramya0820]

Discussed offline, was able to get this PR working fine with npm.
On troubleshooting rush + rollup further in order to get this working on CI, a different set of changes to rollup was required.

Just as we did for #2163, which eventually resulted in us needing more investigation on rush+rollup #3326, we will need work from #3326 to help with troubleshooting EventHubs browser test CI runs as well.

As a summary of findings so far - the problematic references that I was grappling with is mainly Buffer. De-duping seems to make a difference to being able to even locate the references. And 'Buffer' in case of EventHubs seems somewhat tied with 'util'. Needs investigation just like what we did/doing for #3119 / #3326.

cc @ramya-rao-a @AlexGhiondea @mikeharder

[ramya-rao-a]

Please correct me if I am wrong here

• The tests in the PR [EventHubs] Pre-requisites to run browser tests #3445 run fine when using npm locally and this does not need the de-duping
• The de-duping was done in an effort to get the tests to run using rush in CI, which we are not yet able to do.

If the above is true then I suggest to

• remove the de-duping in the PR [EventHubs] Pre-requisites to run browser tests #3445 as they are anyway not helping
• update the script to not run the CI tests for browser
• have the changes in the PR merged
• Update [Service Bus] Long term fix for duplication of module references by rush + node-resolve rollup plugin #3326 to also include all the de-duping you feel are needed to get Event Hubs tests working in CI.

[ramya0820]

Please correct me if I am wrong here

• The tests in the PR [EventHubs] Pre-requisites to run browser tests #3445 run fine when using npm locally and this does not need the de-duping
• The de-duping was done in an effort to get the tests to run using rush in CI, which we are not yet able to do.

If the above is true then I suggest to

• remove the de-duping in the PR [EventHubs] Pre-requisites to run browser tests #3445 as they are anyway not helping
• update the script to not run the CI tests for browser
• have the changes in the PR merged
• Update [Service Bus] Long term fix for duplication of module references by rush + node-resolve rollup plugin #3326 to also include all the de-duping you feel are needed to get Event Hubs tests working in CI.

No, that's not true - de-duping was required to work with npm as well. Current changes are not working with rush inspite of de-duping. Some other workaround may be required which will require same work as being done in #3326.
A different kind of de-duping was getting things with Rush to somewhat work (pass compilation stage), but was still failing overall (runtime).

[ramya-rao-a]

Thanks for the clarification!

Since we are not completely comfortable with the very act of de-duping 1 dependency (Buffer for Service Bus, thus the issue of #3326), I would say let's not introduce 4 more de-dupes until we know more about why rush needs this.

My proposal:

• Updates to PR [EventHubs] Pre-requisites to run browser tests #3445
  • Comment the de-duping part in PR [EventHubs] Pre-requisites to run browser tests #3445 with a comment like needed to run tests in browser or whatever is more accurate
  • Update the integration-test:browser to skip the tests
  • Get the PR merged. This way we have all non rush/npm related changes needed for browser tests checked in.
• Update [Service Bus] Long term fix for duplication of module references by rush + node-resolve rollup plugin #3326 with the findings of what you had to do for Event Hubs. It might be helpful for that investigation.
• [Event Hubs] Add tests for browser mode #2809 will go back to the backlog being blocked on [Service Bus] Long term fix for duplication of module references by rush + node-resolve rollup plugin #3326

[ramya-rao-a]

With #3326 now resolved, we should aim at enabling the browser tests in CI in the current sprint

[chradek]

#5345 Added the final piece for getting browser CI working. Closing!