This repository was archived by the owner on Aug 18, 2021. It is now read-only.

feat(ServiceBus): adds unit tests #197

Open

bruno-brant wants to merge 13 commits into master from feature/add-unit-tests-for-servicebus

Contributor

bruno-brant commented Jan 31, 2020 •

edited

Loading

Should only be approved after #217 and #210.

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch from 0b8c6e7 to 79207fa Compare

January 31, 2020 22:44

Contributor Author

bruno-brant commented Jan 31, 2020

Hey @guilhermeluizsp could I get your input here?

There's a test failing in this PR - it seems that the DLR is failing to resolve (bind) to a property of a dynamic type. Do you have any idea why? What am I missing?

Contributor

guilhermeluizsp commented Feb 1, 2020

@bruno-brant Yeah. The Foobar class is private so its members are only visible to the LightWorkerTests class. Even the default DLR binder does not bypass access modifiers.

We can either make it public to make the binder work correctly, or create a custom DynamicObject to invoke inaccessible members.

Contributor Author

bruno-brant commented Feb 1, 2020

@guilhermeluizsp huh, would've never guessed that was the problem, but surely it's simple enough, I will make the class public.

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch from 79207fa to ba0c280 Compare

February 5, 2020 02:01

bruno-brant changed the title ~~WIP feat: adds unit tests for ServiceBus~~ WIP: feat: adds unit tests for ServiceBus

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch from ba0c280 to ef51b64 Compare

February 7, 2020 21:00

bruno-brant changed the title ~~WIP: feat: adds unit tests for ServiceBus~~ feat: adds unit tests for ServiceBus

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch 4 times, most recently from 1aa2ac3 to f4899e4 Compare

February 7, 2020 21:21

bruno-brant requested a review from guilhermeluizsp

February 7, 2020 21:33

guilhermeluizsp reviewed

View reviewed changes

src/Liquid.OnAzure/MessageBuses/AzureServiceBus.cs Outdated Show resolved Hide resolved

bruno-brant changed the title ~~feat: adds unit tests for ServiceBus~~ WIP: feat: adds unit tests for ServiceBus

bruno-brant requested a review from guilhermeluizsp

February 11, 2020 14:40

bruno-brant added the enhancement label

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch from 228d2e5 to 00dd8d3 Compare

February 11, 2020 21:40

bruno-brant changed the title ~~WIP: feat: adds unit tests for ServiceBus~~ WIP: feat(ServiceBus): adds unit tests

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch 4 times, most recently from 04a2848 to e00db1a Compare

February 14, 2020 14:16

Contributor

guilhermeluizsp commented Feb 14, 2020

@bruno-brant I was supposed to review this PR a long time ago, I'm really sorry for that. May you please rebase with the master branch (so I can begin to review it)? I'm seeing things from previous PRs that were merged already.

Contributor Author

bruno-brant commented Feb 14, 2020

@bruno-brant I was supposed to review this PR a long time ago, I'm really sorry for that. May you please rebase with the master branch (so I can begin to review it)? I'm seeing things from previous PRs that were merged already.

Oh, no worries, it is still work in progress.

You are correct, I probably need to work some git magic here, because I've rebased a number of branches after branching this one...

I will do this shortly. I do want your opinion. I was unable to do testing on ServiceBus without refactoring it, at least a little. And there's a dirty hack on a test that you'll probably despise, but I believe we can live with it until we rewrite this class / the whole LightWorker approach.

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch 2 times, most recently from ec09746 to d78e58e Compare

February 19, 2020 21:22

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch 2 times, most recently from 9263657 to 2c8d7b1 Compare

February 26, 2020 15:24

bruno-brant mentioned this pull request

feat(LightTelemetry): adds TrackException to interface #217

Merged

bruno-brant added 11 commits

February 26, 2020 14:32


          feat(LightTelemetry): adds TrackException to interface

df4a162

ILightTelemetry lacked the TrackException method. This was done by design - to avoid consumer code to call TrackException, it seems, because exception handling was supposed to be done at framework level.

Our take is that this creates more problems than solves. Consumers will still be able to directly use LightTelemetry if they desire, or handle the exception and NOT call TrackException when they should.

Also, that meant a lot of violations of LSP in the framework code, and led to numerous issues during testing since it was impossible to mockout the LightTelemetry.TrackEvent dependency.

This commit adds the method and removes all LSP violations that I could find.

Closes #159.


          fix: inhibits warning

b41a16a


          feat: adds unit tests for LightWorker

bbe63a2


          feat: adds stylecop to Activation


          feat(LightWorker): added more tests conditions

c796359


          feat(LightWorker): more unit tests

cb55540


          feat(LightWorker): added tests to increase coverage

e640b56

Coverage is now above 90%.


          fix(LightWorker): Fixed instancing DomainResponse on tests

8788ca5


          feat(ServiceBus): added unit tests

44a81f7

Added some tests that should test the general behavior of ServiceBus.


          fix: disable parallelization

8cd608c

Parallel tests results in concurrency on Workbench (add vs reset).


          fix: test now validates correct case

fa1142e

bruno-brant force-pushed the feature/add-unit-tests-for-servicebus branch from 2c8d7b1 to fa1142e Compare

February 26, 2020 18:22

bruno-brant added 2 commits

February 26, 2020 15:27


          fix: add missing dependencies

32f846f


          feat(ServiceBus): increases coverage > 90%

ac890d4

bruno-brant changed the title ~~WIP: feat(ServiceBus): adds unit tests~~ feat(ServiceBus): adds unit tests

guilhermeluizsp reviewed

View reviewed changes

src/Liquid.OnAzure/MessageBuses/ServiceBus.cs

+                      /// <returns>
+                      /// The current configuration for a service bus.
+                      /// </returns>
+                      ServiceBusConfiguration GetConfiguration();

Contributor

guilhermeluizsp Mar 2, 2020

Both GetConfiguration and GetConfiguration(string connectionName) will basically do the same thing. I was wondering if we could keep just GetConfiguration(string connectionName) and create an extension method for GetConfiguration() that routes to GetConfiguration(string.Empty), and the implementation takes care of how do handle it (just like Microsoft's IHttpClientFactory does). What do you think?

guilhermeluizsp reviewed

View reviewed changes

src/Liquid.OnAzure/MessageBuses/ServiceBus.cs

                       public Task ExceptionReceivedHandler(ExceptionReceivedEventArgs exceptionReceivedEventArgs)
                       {
                           //Use the class instead of interface because tracking exceptions directly is not supposed to be done outside AMAW (i.e. by the business code)
-                          ((LightTelemetry)Workbench.Instance.Telemetry).TrackException(exceptionReceivedEventArgs.Exception);
+                          Workbench.Instance.Telemetry.TrackException(exceptionReceivedEventArgs.Exception);

Contributor

guilhermeluizsp Mar 2, 2020

I'm not sure if that's the standard we'd like to use, but ServiceBus inherits from LightWorker, which already have a Telemetry property that points exactly to Workbench.Instance.Telemetry. Do you think we should use it or perhaps it is too much of nitpicking of me?

guilhermeluizsp reviewed

View reviewed changes

src/Liquid.OnAzure/MessageBuses/ServiceBus.cs

@@ @@ -96,7 +198,7 @@ public void ProcessQueue() @@
                                           {
                                               Exception moreInfo = new Exception($"Exception reading message from queue {queueName}. See inner exception for details. Message={exRegister.Message}", exRegister);
                                               //Use the class instead of interface because tracking exceptions directly is not supposed to be done outside AMAW (i.e. by the business code)
-                                              ((LightTelemetry)Workbench.Instance.Telemetry).TrackException(moreInfo);
+                                              Workbench.Instance.Telemetry.TrackException(moreInfo);

Contributor

guilhermeluizsp Mar 2, 2020

Same thing here about the Telemetry property from LightWorker.

guilhermeluizsp reviewed

View reviewed changes

src/Liquid.OnAzure/MessageBuses/ServiceBus.cs

@@ @@ -114,7 +216,7 @@ public void ProcessQueue() @@
                           {
                               Exception moreInfo = new Exception($"Error setting up queue consumption from service bus. See inner exception for details. Message={exception.Message}", exception);
                               //Use the class instead of interface because tracking exceptions directly is not supposed to be done outside AMAW (i.e. by the business code)
-                              ((LightTelemetry)Workbench.Instance.Telemetry).TrackException(moreInfo);
+                              Workbench.Instance.Telemetry.TrackException(moreInfo);

Contributor

guilhermeluizsp Mar 2, 2020

And here.

guilhermeluizsp reviewed

View reviewed changes

src/Liquid.OnAzure/MessageBuses/ServiceBus.cs

@@ @@ -155,7 +260,7 @@ private void ProcessSubscription() @@
                                           {
                                               Exception moreInfo = new Exception($"Exception reading message from topic {topicName} and subscriptName {subscriptName}. See inner exception for details. Message={exRegister.Message}", exRegister);
                                               //Use the class instead of interface because tracking exceptions directly is not supposed to be done outside AMAW (i.e. by the business code)
-                                              ((LightTelemetry)Workbench.Instance.Telemetry).TrackException(moreInfo);
+                                              Workbench.Instance.Telemetry.TrackException(moreInfo);

Contributor

guilhermeluizsp Mar 2, 2020

Here too.

guilhermeluizsp reviewed

View reviewed changes

src/Liquid.OnAzure/MessageBuses/ServiceBus.cs

@@ @@ -200,13 +305,57 @@ private void ProcessSubscription() @@
                           {
                               Exception moreInfo = new Exception($"Error setting up subscription consumption from service bus. See inner exception for details. Message={exception.Message}", exception);
                               //Use the class instead of interface because tracking exceptions directly is not supposed to be done outside AMAW (i.e. by the business code)
-                              ((LightTelemetry)Workbench.Instance.Telemetry).TrackException(moreInfo);
+                              Workbench.Instance.Telemetry.TrackException(moreInfo);

Contributor

guilhermeluizsp Mar 2, 2020

And also here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels