Follow up #1670: Fix StyleCop issues and improve the code #1711

raman-m · 2023-09-30T12:22:37Z

Relates to #1670

This is follow up PR after merging of PR #1670

#1634 #1487 #1329 #1304 #1294 #793 Consul polling of services: enhancements and fix errors #1670

Proposed Changes

Reviewed the code of the related PR
Removed code smells
Fixed StyleCop rules violations
Made code improvements

raman-m · 2023-09-30T12:28:10Z

@ggnaegi FYI
As we agreed on PR 1670 merging I've created this PR.

raman-m · 2023-09-30T12:31:15Z

@RaynaldM Hey, Ray!
I will appreciate your review and approval ASAP.
Thank you!

ggnaegi · 2023-09-30T12:39:20Z

@ggnaegi FYI
As we agreed on PR 1670 merging I've created this PR.

@raman-m I could review it...Because most of it is not my code.

raman-m · 2023-09-30T12:45:13Z

@ggnaegi Your approval will have gray status which means it does not approve the PR actually and uncover PR status to merge.
But your opinion is welcomed! I could even fix something if you judge my code 😉

raman-m · 2023-10-01T08:59:27Z

@ggnaegi Will you review?
Is it Okay to merge?

ggnaegi · 2023-10-01T09:00:51Z

@ggnaegi Will you review?

@raman, yes sure, probably in the afternoon...

ggnaegi

@raman-m I reviewed the code:

1. ServiceValidationWarningFormat -> I would use a static factory method, otherwise it's error prone
1. ThenTheLoggerHasBeenCalledCorrectlyWithValidationWarning -> I would implement only one theory instead of 3 facts, otherwise we would have to verify the port or url explicitely to match the fact name
1. My bad, The theory and facts names are wrong -> should_return_provider_according_to_service_name

src/Ocelot.Provider.Consul/ConsulRegistryConfiguration.cs

src/Ocelot.Provider.Consul/UnableToSetConfigInConsulError.cs

src/Ocelot.Provider.Eureka/EurekaProviderFactory.cs

ggnaegi · 2023-10-01T12:37:33Z

test/Ocelot.UnitTests/Consul/ConsulServiceDiscoveryProviderTests.cs

@@ -193,47 +193,18 @@ public void should_not_return_services_with_invalid_port()
                .And(x => GivenTheServicesAreRegisteredWithConsul(serviceEntryOne, serviceEntryTwo))
                .When(x => WhenIGetTheServices())
                .Then(x => ThenTheCountIs(0))
-                .And(x => ThenTheLoggerHasBeenCalledCorrectlyForInvalidPorts())
+                .And(x => ThenTheLoggerHasBeenCalledCorrectlyWithValidationWarning(serviceEntryOne, serviceEntryTwo))


We could go a step further here and implement a theory instead of 3 facts, otherwise I would expect an explicit port or url validation.

A-ha-ha! It seems I found the bug made by Tom in 11a2d13 on line 61...
If address contains http OR address contains https then it is actually valid but it returns false 🤣

SO, assumption in the should_not_return_services_with_invalid_address test is wrong because Address = "http://localhost" is valid, but the test considers it as invalid because of the bug in boolean formula of the predicate. 🥸
Okay... Tom... Good job! 🤣

Update 1

Sorry, my bad!
localhost is valid, but if scheme in a string like http://localhost then it is invalid. This is the requirement of Consul AgentService class. So, not an issue. The test is correct.
So, I was confused by Address, for AgentService it is host actually.

@ggnaegi Partially fixed in 5851300

Regarding Theory... You will be able to refactor these 3 tests as you wish in the next PRs... 😉
Currently I cannot merge 3 tests to one theory because of closure of _serviceName private member while AgentService creating, but MemberData attribute of Theory requires only static property/member and it has no access to object members! So, it will be a little challenge for you. 😄

And version you've reviewed just a refactoring, I test the same for which these tests were written. So, port & service name checking is encapsulated inside of the warning expected string.
So, writing something like actualWarning.ShouldContain(portString) will be overhead checks.
Finally, I believe, it is not an issue.

@raman-m Ok 😄

ggnaegi · 2023-10-01T12:53:50Z

test/Ocelot.UnitTests/Consul/ProviderFactoryTests.cs

    {
        var provider = DummyPollingConsulServiceFactory(string.Empty);
        var pollProvider = provider as PollConsul;
        pollProvider.ShouldNotBeNull();
    }

    [Fact]
-    public void should_return_SameProviderForGivenServiceName()
+    public void ShouldReturnSameProviderForGivenServiceName()


I think here it should be should_return_same_provider_for_given_service_name, it matches the naming conventions.

Hmm... BDDfy can work with different name conventions: "You can use underscored or pascal or camel cased method names for your steps"
But the C# naming convention is PascalCase. As for C# developer, as for me, it is very strange to see C & C++ naming convention with _ (underscores) in method names!
In future I am going to fix this huge problem.

2nd problem. Visual Studio generates a lot of IDE1006 Naming rule violation because method name is not PascalCase.
But for now, it seems scenarios with PascalCase is not logged into build log at all. Probably it is Ok because passed scenario should not be logged by default.
Currently all BDDfy unit tests are logged producing huge logs... But this is another story.

It makes a lot of sense to change names from underscore-style (C++) to C# PascalCase because of the following:

Passed unit test will not be logged by BDDfy framework to build console on CircleCI

Method names of unit tests don't violate default C# naming conventions (aka IDE1006 rule, etc.)

@raman-m Ok, Raman it makes sense: So, we could keep it like that and create a new PR for theories / facts naming conventions. I'm not used to the underscore naming convention either.

✔️ Fixed in 842a941

It makes too much sense... But not in this PR which I would like to merge today :)
It will another huge PR to resolve mentioned problems... in October from me... 😸

It seems I must create another follow up issue after this follow up... 🤣 I'm going crazy with this project...

test/Ocelot.UnitTests/Consul/ProviderFactoryTests.cs

src/Ocelot.Provider.Consul/Consul.cs

raman-m · 2023-10-02T08:10:35Z

@RaynaldM Sorry, there will be a number of code review commits... Wait for my next message please!

Create expected value from a hardcoded string in unit tests

raman-m · 2023-10-02T14:00:07Z

@ggnaegi I've resolved issues of your code review!
Are we done?

raman-m · 2023-10-02T14:03:35Z

@RaynaldM I'm sorry that I dismissed your approvals multiple times!
Make a final approval, plz 🙏

ggnaegi

Yes!

raman-m · 2023-10-04T16:23:05Z

4 ⭐⭐⭐⭐ to Raman Maksimchuk 🥇
3 ⭐⭐⭐ to jlukawska 🥈
2 ⭐⭐ to Guillaume Gnaegi 🥉

Honoring 🏅 aka Top Contributors 👏

1st 🥇 goes to Raman Maksimchuk for delivering 4 commits
2nd 🥈 goes to jlukawska for delivering 3 commits
3rd 🥉 goes to Guillaume Gnaegi for delivering 2 commits in 2 files changed
3rd 🥉 goes to Ben Bartholomew for delivering 1 feature in 3 files changed with 83 insertions
3^rd 🥉 goes to Guillaume Gnaegi for delivering 1 feature in 3 files changed with 13 insertions and with 422 deletions

Starring ⭐ aka Release Influencers

⭐⭐⭐⭐ Raman Maksimchuk
⭐⭐⭐ jlukawska
⭐⭐ Guillaume Gnaegi
⭐ Anthony Steele
⭐ Ben Bartholomew
⭐ DanHarltey
⭐ Fabrizio Mancin
⭐ Freddy
⭐ Kevin Grossmann
⭐ Marco, Roman, Thiago Loureiro, Vijay Karavadra, guoyongchang, leonluc-dev, raman-m, zhangq

1^st 🥇 goes to DanHarltey for delivering 1 feature in 8 files changed
2^nd 🥈 goes to Ben Bartholomew for delivering 1 feature in 3 files changed with 83 insertions
3^rd 🥉 goes to Guillaume Gnaegi for delivering 1 feature in 3 files changed with 13 insertions and 422 deletions

raman-m · 2023-10-05T10:36:42Z

September 2023 (version 19.1.0)

Honoring 🏅 aka Top Contributors 👏

1^st 🥇 goes to raman-m for delivering 7 features
2^nd 🥈 goes to Raman Maksimchuk for delivering 4 features
3^rd 🥉 goes to jlukawska for delivering 3 features

Starring ⭐ aka Release Influencers

⭐⭐⭐⭐⭐⭐⭐ raman-m
⭐⭐⭐⭐ Raman Maksimchuk
⭐⭐⭐ jlukawska
⭐⭐ Guillaume Gnaegi
⭐ Anthony Steele
⭐ Ben Bartholomew
⭐ DanHarltey
⭐ Fabrizio Mancin
⭐ Freddy
⭐ Kevin Grossmann
⭐ Marco
⭐ Roman
⭐ Thiago Loureiro
⭐ Vijay Karavadra
⭐ guoyongchang
⭐ leonluc-dev
⭐ zhangq

Features in the Release

deleted

raman-m requested a review from RaynaldM September 30, 2023 12:23

raman-m self-assigned this Sep 30, 2023

raman-m added good first issue Should be pretty easy to do proposal Proposal for a new functionality in Ocelot Service Discovery Ocelot feature: Service Discovery labels Sep 30, 2023

raman-m changed the title ~~Follow up #1670: Remove code smells and improve the code~~ Follow up #1670: Fix StyleCop issues and improve the code Sep 30, 2023

raman-m force-pushed the bug-1634-followup-of-PR-1670 branch from 9f560b0 to 44d5a5a Compare September 30, 2023 14:33

raman-m added 2 commits September 30, 2023 17:47

Code refactoring and improvements

c3fb9cf

Fix unit tests for ConsulServiceDiscoveryProvider

b905fde

raman-m force-pushed the bug-1634-followup-of-PR-1670 branch from 44d5a5a to b905fde Compare September 30, 2023 14:48

raman-m added 2 commits September 30, 2023 20:39

Merge branch 'develop' into bug-1634-followup-of-PR-1670

15adf3d

Merge branch 'develop' into bug-1634-followup-of-PR-1670

521f898

ggnaegi requested changes Oct 1, 2023

View reviewed changes

RaynaldM previously approved these changes Oct 2, 2023

View reviewed changes

Add default port

679c4ff

raman-m dismissed RaynaldM’s stale review via 679c4ff October 2, 2023 07:51

RaynaldM previously approved these changes Oct 2, 2023

View reviewed changes

Throw NullRef exception if no client

ef754e1

raman-m dismissed RaynaldM’s stale review via ef754e1 October 2, 2023 08:09

Fix 'should_not_get' unit test

39fd538

RaynaldM previously approved these changes Oct 2, 2023

View reviewed changes

Add AgentService extensions

5851300

raman-m dismissed RaynaldM’s stale review via 5851300 October 2, 2023 12:07

raman-m added 3 commits October 2, 2023 16:05

Use current naming conventions for unit tests

842a941

Fix code smell with methos signature

ec84a0c

Encapsulate warning string creation.

eefa042

Create expected value from a hardcoded string in unit tests

raman-m requested a review from ggnaegi October 2, 2023 13:59

raman-m requested a review from RaynaldM October 2, 2023 14:01

ggnaegi approved these changes Oct 2, 2023

View reviewed changes

RaynaldM approved these changes Oct 2, 2023

View reviewed changes

raman-m merged commit e5f31ef into develop Oct 2, 2023
2 checks passed

raman-m deleted the bug-1634-followup-of-PR-1670 branch October 2, 2023 16:18

raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed good first issue Should be pretty easy to do labels Oct 2, 2023

raman-m mentioned this pull request Oct 2, 2023

#1708 Generic polling mechanism for service discovery #1710

Closed

raman-m mentioned this pull request Oct 5, 2023

A brand new Release Notes feature for Releases being made by 'build.cake' script #1716

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up #1670: Fix StyleCop issues and improve the code #1711

Follow up #1670: Fix StyleCop issues and improve the code #1711

raman-m commented Sep 30, 2023 •

edited

Loading

raman-m commented Sep 30, 2023

raman-m commented Sep 30, 2023 •

edited

Loading

ggnaegi commented Sep 30, 2023 •

edited

Loading

raman-m commented Sep 30, 2023

raman-m commented Oct 1, 2023 •

edited

Loading

ggnaegi commented Oct 1, 2023

ggnaegi left a comment •

edited

Loading

ggnaegi Oct 1, 2023

raman-m Oct 2, 2023 •

edited

Loading

raman-m Oct 2, 2023 •

edited

Loading

ggnaegi Oct 2, 2023

ggnaegi Oct 1, 2023

raman-m Oct 2, 2023

ggnaegi Oct 2, 2023 •

edited

Loading

raman-m Oct 2, 2023

raman-m Oct 2, 2023

raman-m commented Oct 2, 2023

raman-m commented Oct 2, 2023 •

edited

Loading

raman-m commented Oct 2, 2023

ggnaegi left a comment

raman-m commented Oct 4, 2023

raman-m commented Oct 5, 2023 •

edited

Loading

Follow up #1670: Fix StyleCop issues and improve the code #1711

Follow up #1670: Fix StyleCop issues and improve the code #1711

Conversation

raman-m commented Sep 30, 2023 • edited Loading

Relates to #1670

Proposed Changes

raman-m commented Sep 30, 2023

raman-m commented Sep 30, 2023 • edited Loading

ggnaegi commented Sep 30, 2023 • edited Loading

raman-m commented Sep 30, 2023

raman-m commented Oct 1, 2023 • edited Loading

ggnaegi commented Oct 1, 2023

ggnaegi left a comment • edited Loading

Choose a reason for hiding this comment

ggnaegi Oct 1, 2023

Choose a reason for hiding this comment

raman-m Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Update 1

raman-m Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

ggnaegi Oct 2, 2023

Choose a reason for hiding this comment

ggnaegi Oct 1, 2023

Choose a reason for hiding this comment

raman-m Oct 2, 2023

Choose a reason for hiding this comment

ggnaegi Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

raman-m Oct 2, 2023

Choose a reason for hiding this comment

raman-m Oct 2, 2023

Choose a reason for hiding this comment

raman-m commented Oct 2, 2023

raman-m commented Oct 2, 2023 • edited Loading

raman-m commented Oct 2, 2023

ggnaegi left a comment

Choose a reason for hiding this comment

raman-m commented Oct 4, 2023

Honoring 🏅 aka Top Contributors 👏

Starring ⭐ aka Release Influencers

raman-m commented Oct 5, 2023 • edited Loading

September 2023 (version 19.1.0)

Honoring 🏅 aka Top Contributors 👏

Starring ⭐ aka Release Influencers

Features in the Release

raman-m commented Sep 30, 2023 •

edited

Loading

raman-m commented Sep 30, 2023 •

edited

Loading

ggnaegi commented Sep 30, 2023 •

edited

Loading

raman-m commented Oct 1, 2023 •

edited

Loading

ggnaegi left a comment •

edited

Loading

raman-m Oct 2, 2023 •

edited

Loading

raman-m Oct 2, 2023 •

edited

Loading

ggnaegi Oct 2, 2023 •

edited

Loading

raman-m commented Oct 2, 2023 •

edited

Loading

raman-m commented Oct 5, 2023 •

edited

Loading