Create public contructors for StackExchange.Redis event args for testing purpose #1326

n1l · 2020-01-21T11:46:50Z

Create public contructors for StackExchange.Redis event args to be able to create tests scenarios based on redis events. Add sealed attribute to other EventArgs for consistency.

Issue with details - #1325

mgravell · 2020-01-21T12:55:34Z

src/StackExchange.Redis/HashSlotMovedEventArgs.cs

+        /// <param name="hashSlot">Hash slot.</param>
+        /// <param name="old">Old endpoint.</param>
+        /// <param name="new">New endpoint.</param>
+        public HashSlotMovedEventArgs(EventHandler<HashSlotMovedEventArgs> handler, object sender,


this is a very unusual API, for specific reasons; I don't think any external caller (for unit testing etc) would need this - or indeed could use this; I wonder if the public constructor should just be:

public HashSlotMovedEventArgs(int hashSlot, EndPoint old, EndPoint @new) : this (null, null, hashSlot, old, new) {}

thoughts?

Still this is a public event and someone may be interested to unit-test some scenarios. For testing purpose I don't think the c'tor should be different.

My answer to that, then: is show the scenario that needs that, ideally in a test. Adding something without a demonstrated use-case is ... not a good way to design the API usually.

@mgravell this is an outdated comment. I've updated a pr. So yes, I agree with you, handler is no need for this.

mgravell · 2020-01-21T12:56:51Z

src/StackExchange.Redis/ConnectionFailedEventArgs.cs

+         /// <param name="failureType">Redis connection failure type.</param>
+         /// <param name="exception">The exception occured.</param>
+         /// <param name="physicalName">Connection physical name.</param>
+        public ConnectionFailedEventArgs(EventHandler<ConnectionFailedEventArgs> handler, object sender, EndPoint endPoint, ConnectionType connectionType, ConnectionFailureType failureType, Exception exception, string physicalName)


similar to the comment on EndPointEventArgs - I wonder if the public .ctor should not expose handler / sender

Answer is the same.

mgravell · 2020-01-21T12:57:46Z

src/StackExchange.Redis/InternalErrorEventArgs.cs

+        /// <param name="connectionType">Redis connection type.</param>
+        /// <param name="exception">The exception occured.</param>
+        /// <param name="origin">Origin.</param>
+        public InternalErrorEventArgs(EventHandler<InternalErrorEventArgs> handler, object sender, EndPoint endpoint, ConnectionType connectionType, Exception exception, string origin)


mgravell · 2020-01-21T12:57:52Z

src/StackExchange.Redis/RedisErrorEventArgs.cs

+        /// <param name="sender">The source of the event.</param>
+        /// <param name="endpoint">Redis endpoint.</param>
+        /// <param name="message">Error message.</param>
+        public RedisErrorEventArgs(


mgravell · 2020-01-21T12:59:51Z

The very unusual construction API here is because of the way that ICompletable works; I can't see any unit-test scenarios where that would be necessary/appropriate. Now, this could just be that I'm being unimaginative, but... I suspect we can drop those and leave more idiomatic APIs.

In particular, I don't want to force us to make an implementation detail part of the public API.

Thoughts and discussion welcome.

Also: it would really help to see these how they are intended to be used, i.e. in a unit test that uses them how you want to.

n1l

@mgravell ok let me create some samples for example

n1l · 2020-01-21T13:15:11Z

Oh, mb you're talking about public ctor's without params at all? Like for testing purpose?

mgravell · 2020-01-21T13:18:28Z

Oh, mb you're talking about public ctor's without params at all? Like for testing purpose?

no, it is just the handler and sender that are atypical; the API here reverses that (because: reasons). You would not usually have those as parameters on classic event args. So I'm saying: if the args has those: create new public .ctors that invoke the existing internal .ctors, passing null for handler and args, but forward onwards any others. If the old .ctor only has those parameters, then yes: that would result in a parameterless constructor.

mgravell · 2020-01-21T13:19:27Z

see also: the example I gave for HashSlotMovedEventArgs

n1l · 2020-01-21T13:22:48Z

@mgravell
Yes I've understood the idea a bit later :)

But still I think I need sender becase I want to be aware of the type of the library raised the event. So only handler is not need.

n1l · 2020-01-21T13:34:02Z

@mgravell I've updated please take a look.

n1l · 2020-01-21T14:07:06Z

@mgravell Ahh, ok it's not a big deal. Also there are some fails on linux.

…le to create tests scenarios based on redis events.

n1l · 2020-01-21T14:32:23Z

@mgravell please take a look again :)

n1l · 2020-01-22T10:36:35Z

@mgravell is everything ok? :)

mgravell · 2020-01-22T11:06:24Z

I know you don't mean anything by it, but keep in mind that OSS maintainers aren't around 24/7 to jump on reviews - lost of plates spinning, including jobs, family, sleep, etc; I plan on taking another look as soon as I have a moment, probably later today.

n1l · 2020-01-22T16:17:14Z

@mgravell Thank you! :)

NickCraver · 2020-01-26T01:53:20Z

Minor language note here, could you please change all descriptions from "The constructor for testing purpose." to "This constructor is only for testing purposes."?

n1l · 2020-01-27T08:09:23Z

This constructor is only for testing purposes."?

sure!

mgravell · 2020-01-27T10:18:55Z

Constructors look good, but there's still nothing test-wise that indicates the intended usage. What makes me nervous is that as a general principle of API design, I usually prefer to see an example usage along with any new API (including new methods/overloads); that's usually how we discover whether the proposed API actually achieves the things we want to demonstrate. Is there any way of adding a meaningful test here? Not just "oh look, I can call a constructor" - but rather: something that actually shows why we're adding it, and that we have achieved what we wanted. If that means adding Moq or whatever to the test project, I'm fine with that...

n1l · 2020-01-27T11:33:47Z

Is there any way of adding a meaningful test here?

Yes of course, I was just a bit lazy to add them because I thought it's obvious, but ok it's not a big deal again.

Not just "oh look, I can call a constructor" - but rather: something that actually shows why we're adding it, and that we have achieved what we wanted.

Do you mean something except tests(documentation, comments samples) or only tests?

mgravell · 2020-01-27T12:10:58Z

Do you mean something except tests(documentation, comments samples) or only tests?

I'm just talking about tests here; this isn't an API that needs documentation - just: you're adding this API because you want to do a thing; it would be good to see the thing happening, so we can prove that it is the right API and enables the thing.

n1l · 2020-02-01T16:30:26Z

Hi, @NickCraver , @mgravell !
I've added some tests for the feature. I've removed sealed attribute because substitute mocking framework can't create proxy for a sealed class. Hope it's ok.

So the Idea is - if I have a component which can do some diagnostic for me(logging, pushing events etc) I want to tests it's handlers. But if there is no public c'tors for event args I can't invoke such handlers and test my component.

mgravell · 2020-02-01T18:38:09Z

Awesome. Will look!

…

On Sat, 1 Feb 2020, 16:30 n1l, ***@***.***> wrote: Hi, @NickCraver <https://github.com/NickCraver> , @mgravell <https://github.com/mgravell> ! I've added some tests for the feature. I've removed sealed attribute because substitute mocking framework can't create proxy for a sealed class. Hope it's ok. So the Idea is - if I have a component which can do some diagnostic for me(logging, pushing events etc) I want to tests it's handlers. But if there is no public c'tors for event args I can't invoke such handlers and test my component. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1326?email_source=notifications&email_token=AAAEHMBRAOQ65H3SY6MZYQ3RAWPSHA5CNFSM4KJSCLM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKRA53I#issuecomment-581045997>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMB2S772CCG6ECDBXP3RAWPSHANCNFSM4KJSCLMQ> .

mgravell · 2020-02-03T07:26:16Z

Merging, thanks

mgravell reviewed Jan 21, 2020

View reviewed changes

n1l commented Jan 21, 2020

View reviewed changes

n1l force-pushed the feature/public-ctor-eventargs branch from f2ff868 to 7088227 Compare January 21, 2020 13:33

Create public contructors for StackExchange.Redis event args to be ab…

7a2314b

…le to create tests scenarios based on redis events.

n1l force-pushed the feature/public-ctor-eventargs branch from 1091383 to 7a2314b Compare January 21, 2020 14:30

Update c'tors description

385db54

Update event args, create tests

eac0f66

mgravell merged commit 162f2b4 into StackExchange:master Feb 3, 2020

Create public contructors for StackExchange.Redis event args for testing purpose #1326

Create public contructors for StackExchange.Redis event args for testing purpose #1326

Uh oh!

Conversation

n1l commented Jan 21, 2020

Uh oh!

mgravell Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

n1l Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgravell Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

n1l Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgravell Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

n1l Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

mgravell Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

mgravell Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

mgravell commented Jan 21, 2020

Uh oh!

n1l left a comment

Choose a reason for hiding this comment

Uh oh!

n1l commented Jan 21, 2020

Uh oh!

mgravell commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgravell commented Jan 21, 2020

Uh oh!

n1l commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1l commented Jan 21, 2020

Uh oh!

n1l commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1l commented Jan 21, 2020

Uh oh!

n1l commented Jan 22, 2020

Uh oh!

mgravell commented Jan 22, 2020

Uh oh!

n1l commented Jan 22, 2020

Uh oh!

NickCraver commented Jan 26, 2020

Uh oh!

n1l commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgravell commented Jan 27, 2020

Uh oh!

n1l commented Jan 27, 2020

Uh oh!

mgravell commented Jan 27, 2020

Uh oh!

n1l commented Feb 1, 2020

Uh oh!

mgravell commented Feb 1, 2020 via email

Uh oh!

mgravell commented Feb 3, 2020

Uh oh!

Reviewers

Assignees

Labels

n1l Jan 21, 2020 •

edited

Loading

n1l Jan 21, 2020 •

edited

Loading

mgravell commented Jan 21, 2020 •

edited

Loading

n1l commented Jan 21, 2020 •

edited

Loading

n1l commented Jan 21, 2020 •

edited

Loading

n1l commented Jan 27, 2020 •

edited

Loading