Create public contructors for StackExchange.Redis event args for testing purpose by n1l · Pull Request #1326 · StackExchange/StackExchange.Redis (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
| /// Hash slot. |
|---|
| /// Old endpoint. |
| /// New endpoint. |
| public HashSlotMovedEventArgs(EventHandler<HashSlotMovedEventArgs> handler, object sender, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgravell this is an outdated comment. I've updated a pr. So yes, I agree with you, handler is no need for this.
| /// Redis connection failure type. |
|---|
| /// The exception occured. |
| /// Connection physical name. |
| public ConnectionFailedEventArgs(EventHandler<ConnectionFailedEventArgs> handler, object sender, EndPoint endPoint, ConnectionType connectionType, ConnectionFailureType failureType, Exception exception, string physicalName) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to the comment on EndPointEventArgs - I wonder if the public .ctor should not expose handler / sender
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answer is the same.
| /// Redis connection type. |
|---|
| /// The exception occured. |
| /// Origin. |
| public InternalErrorEventArgs(EventHandler<InternalErrorEventArgs> handler, object sender, EndPoint endpoint, ConnectionType connectionType, Exception exception, string origin) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| /// The source of the event. |
|---|
| /// Redis endpoint. |
| /// Error message. |
| public RedisErrorEventArgs( |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgravell ok let me create some samples for example
Oh, mb you're talking about public ctor's without params at all? Like for testing purpose?
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.
see also: the example I gave for HashSlotMovedEventArgs
n1l commented
• Loading
@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 commented
• Loading
@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.
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.
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 commented
• Loading
This constructor is only for testing purposes."?
sure!
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...
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?
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.
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.
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> .