Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented rule to avoid duplicates #36

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mbauerdev
Copy link

The current Thor implementation silently swallows all events that are passed into an event-method that has a non-unique event identifier. The changes in this PR should detect this issue within an event source:

    [EventSource(Name = "NonUniqueEventId")]
    public class NonUniqueEventIdEventSource
        : EventSource
    {
        [Event(1)] // <-- Duplicate here...
        public void Foo1a(string bar)
        {
            WriteEvent(1, bar);
        }

        [Event(1)] // <-- Duplicate here...
        public void Foo1b(string bar)
        {
            WriteEvent(1, bar);
        }
}

@swisslife-bot
Copy link

swisslife-bot commented Oct 19, 2021

CLA assistant check
All committers have signed the CLA.

Copy link

@joslat joslat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I am wrong and those tests are not used as tests but as rules, so then all is good. Except some spacing, or lines before the return ;) - but that's not serious as here there is no SonarQbe...

}

[Fact(DisplayName = "Apply: Should return an error if event source has non-unique event identifiers")]
public void Apply_NonUniqueEventId()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to have one of the naming conventions (or similar) on the test mehtods. even better when the project's test methods naming are consisten, like "MethodName_StateUnderTest_ExpectedBehavior" which is the one I usually use... with method name I putthe action performed like "ApplyingNonUniqueEventId_ToAnEventSource_ShouldRaiseAnError" for example.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point - I usually follow that rule. But in this case, I decided to have consistency over "correctness"... All other tests follow the same naming pattern.


public static bool IsEvent(this MethodInfo method, int eventId)
{
EventAttribute attribute = method.GetEvent();
return method.GetCustomAttribute<EventAttribute>() != null &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like a return(line) before the return, used to that as this is on other methods too. but haven't seen the rest of code, depends if this is consistent with the rest of the code...


private object GetDuplicatesString(Dictionary<int, string[]> duplicates)
{

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove this

{
result += $"{dup.Key} in [{string.Join(", ", dup.Value)}] ";
}
return result;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or add the above line before this return ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants