-
Notifications
You must be signed in to change notification settings - Fork 46
Tests are failing when using the latest amqp-common library #179
Comments
@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? |
@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 jeremymeng@16a5c5f 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? |
Actually, that particular test case has been breaking every since version The user expectation is that when the connection string doesn't have the entity path, they get the We can update the test case to use |
yes after Azure/amqp-common-js@a2a8e7d#diff-ad0ce2742650d03b78af445d57e30269
Yeah I did that originally. Then I looked further in the code and found that
EventHubConnectionConfig behavior. That's the reason why I want to remove the dead code and this test.
|
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. |
But the error thrown by the below code is not the one being tested and also, I believe it has always been dead code. azure-event-hubs-node/client/lib/eventHubClient.ts Lines 307 to 308 in dff1079
The error thrown by amqp-common is the one being tested. This is the failing test: azure-event-hubs-node/client/tests/client.spec.ts Lines 49 to 54 in 3c865b4
Therefore, changing the input in the test case to |
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. |
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. |
I will do both. I added a new test to amqp-common in PR Azure/amqp-common-js#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. |
actually a similar case is already covered there. so we are all good. |
Describe the bug
Tests are failing when using the latest amqp-common library
To Reproduce
In the
client
folder of this repo,.env
file, setEVENTHUB_CONNECTION_STRING
andEVENTHUB_NAME
variablespackage.json
file to use the latest version of the amqp-common librarynpm install
unit
script to run just theclient.spec.ts
test filenpm run unit
Expected behavior
All tests pass
Actual behavior
Now change the
unit
script to run just thereceiver.spec.ts
. It fails as below:The text was updated successfully, but these errors were encountered: