feat: add initial eventhubs test components#265
feat: add initial eventhubs test components#265stijnmoreels wants to merge 26 commits intoarcus-azure:mainfrom
Conversation
✅ Deploy Preview for arcus-testing canceled.
|
🧪 Code coverage summary
Great job! 😎 Your code coverage is higher than my caffeine levels! ☕ |
…b.com/stijnmoreels/arcus.testing into feature/add-eventhubs-test-components
|
stijnmoreels
added
feature
Use a read-only variant for the `ToList` result of the Azure Service Bus filtering. Follow-up of #265 where @fgheysels made my brain 🧠 work.
fgheysels
left a comment
fgheysels
left a comment
There was a problem hiding this comment.
I just have some suggestions
Co-authored-by: Frederik Gheysels <frederik.gheysels@telenet.be>
Co-authored-by: Frederik Gheysels <frederik.gheysels@telenet.be>
| ArgumentNullException.ThrowIfNull(eventHubsNamespaceResourceId); | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(consumerGroup); | ||
|
|
||
| var credential = new DefaultAzureCredential(); |
There was a problem hiding this comment.
I have some doubts on this approach.
I don't like the fact that we're instantiating a DefaultAzureCredential ourselves here. Wouldn't it be better if we pass in the ITokenCredential that must b e used as a parameter ?
(Using DefaultAzureCredential gives some overhead as well, as multiple auth mechanisms will be tried)
There was a problem hiding this comment.
- Remember that this is also used a lot locally, where you expect to go over the local VS, VS Code, Browser settings. Then using that default credential is a perfect fit for local development/testing.
- For every test fixture that we provide, there is always an overload to pass in the client yourself or by using a different authentication protocol/approach.
| /// Sets up an existing Azure Event Hub. | ||
| /// </summary> | ||
| /// <returns>The name of the newly set up Azure Event Hub.</returns> | ||
| public async Task<string> WhenHubAvailableAsync() |
There was a problem hiding this comment.
I think the name of the method is a bit confusing ?
When looking at the code, this looks like a method that 'blocks' until the eventhub is available.
Maybe the name can be changed to WaitUntilHubAvailableAsync ?
/just my opinion :P
There was a problem hiding this comment.
Ah, and now I see the method below (WhenHubNotAvailable). So these are 2 factory-like methods that are called depending whether you're testing against Azure or not ?
There was a problem hiding this comment.
I think the name of the method is a bit confusing ?
When looking at the code, this looks like a method that 'blocks' until the eventhub is available.
Maybe the name can be changed to WaitUntilHubAvailableAsync ?
If we would include 'wait until ...' in the name, we would be exposing internal infrastructure implementation details to the test body. The test scenario is only concerned about having a hub available or not, not 'how' or 'when' that hub becomes available.
Ah, and now I see the method below (WhenHubNotAvailable). So these are 2 factory-like methods that are called depending whether you're testing against Azure or not ?
It is to give a clear indication in the test bodies that we're testing against a remotely-available Azure Event Hub or not, yes. Without exposing to the test body 'how' or 'when' that is done.
3 participants
Adds initial testing components for interaction with Azure Event Hubs.
Relates to #90