Tests are failing when using the latest amqp-common library

[ramya-rao-a]

Describe the bug Tests are failing when using the latest amqp-common library

To Reproduce In the client folder of this repo,

1. Add a .env file, set EVENTHUB_CONNECTION_STRING and EVENTHUB_NAME variables
2. Update the package.json file to use the latest version of the amqp-common library
3. Run npm install
4. Update the unit script to run just the client.spec.ts test file
5. Run npm run unit

Expected behavior All tests pass

Actual behavior

Now change the unit script to run just the receiver.spec.ts. It fails as below:

[ramya-rao-a]

@jeremymeng Can we close this issue since your work in #187 is now in? Or are there any other failures when using the latest amqp-common module?

[jeremymeng]

@ramya-rao-a We can close this issue unless we are going to upgrade to the latest amqp-common in this repo.

The first failure in your screenshot is not address by this PR. I removed the test in https://github.com/jeremymeng/azure-event-hubs-node/commit/16a5c5fc26d404c0509ebddde2193bd31e61af57 as the code path will no longer be hit with the latest amqp-common. The change will be in the central repo once merged.

This is probably a breaking change caused by amqp-common if consumers rely on the call stack of the thrown error. Do we have a place where we track breaking changes?

[ramya-rao-a]

Actually, that particular test case has been breaking every since version 0.1.4 of amqp common with the error: Cannot read property 'endsWith' of undefined.

The user expectation is that when the connection string doesn't have the entity path, they get the Either provide "path" or the "connectionString": ... must contain EntityPath.... And this still holds true.

We can update the test case to use Endpoint=sb://abc; i.e connection string has endpoint but no entity path instead of abc and close this issue

[jeremymeng]

Actually, that particular test case has been breaking every since version 0.1.4 of amqp common

yes after https://github.com/Azure/amqp-common-js/commit/a2a8e7d5b242d278dbe9e198d07194c32e3d24cf#diff-ad0ce2742650d03b78af445d57e30269

We can update the test case to use Endpoint=sb://abc;

Yeah I did that originally. Then I looked further in the code and found that https://github.com/Azure/azure-event-hubs-node/blob/dff107968b4e97e3786b5fd796464f30c7be6f37/client/lib/eventHubClient.ts#L307 is dead code since an error is already thrown if the entity path is missing, and we are just testing EventHubConnectionConfig behavior. That's the reason why I want to remove the dead code and this test.

[ramya-rao-a]

is dead code since an error is already thrown if the entity path is missing,

Can you point me to the place where this error is thrown?

[jeremymeng]

Can you point me to the place where this error is thrown?

Using amqp-common 0.1.4 or later: https://github.com/Azure/amqp-common-js/blob/c365c92aaa1b718a59859e2ec4a4361c6a68ae41/lib/connectionConfig/eventhubConnectionConfig.ts#L82-L85

Current master is using 0.1.3 so this test is not failing. So if we are not ugrading amqp-common in this repo we can just close this issue.

[ramya-rao-a]

But the error thrown by the below code is not the one being tested and also, I believe it has always been dead code. https://github.com/Azure/azure-event-hubs-node/blob/dff107968b4e97e3786b5fd796464f30c7be6f37/client/lib/eventHubClient.ts#L307-L308

The error thrown by amqp-common is the one being tested. https://github.com/Azure/amqp-common-js/blob/c365c92aaa1b718a59859e2ec4a4361c6a68ae41/lib/connectionConfig/eventhubConnectionConfig.ts#L82-L85

This is the failing test: https://github.com/Azure/azure-event-hubs-node/blob/3c865b47be659071a456e1a3b290eeb650a4ee3f/client/tests/client.spec.ts#L49-L54

Therefore, changing the input in the test case to Endpoint=sb://abc; should work for all versions of amqp-common

[jeremymeng]

My point was that after removing the dead code, there isn't any logic in eventHubClient to test. But I must admit that I was thinking in the unit testing point of view. As an integration test this still has value.

[ramya-rao-a]

Yes, we can either change the input value here to get this test passing or do the same and move this test altogether into amqp-common.

[jeremymeng]

I will do both. I added a new test to amqp-common in PR https://github.com/Azure/amqp-common-js/pull/28 when I removed the scenario in this repo. I will add it back with the updated endpoint.

I will probably add another test with "abc" endpoint to the amqp-common repro.

[jeremymeng]

I will probably add another test with "abc" endpoint to the amqp-common repro.

actually a similar case is already covered there. so we are all good.