-
Notifications
You must be signed in to change notification settings - Fork 112
Added interfaces to allow for mocks in unit testing #11
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
base: master
Are you sure you want to change the base?
Conversation
SimpleTCP/SimpleTcpClient.cs
Outdated
Message WriteLineAndGetReply(string data, TimeSpan timeout); | ||
} | ||
|
||
public class SimpleTcpClient : ISimpleTcpClient, IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd pull the IDisposable inheritance into the ISimpleTcpClient interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree!
Haven't reviewed yet - you guys know if this breaks any existing usage (is this a major or minor rev)? |
Any idea when this PR is going to be merged and released? |
bool AutoTrimStrings { get; set; } | ||
SimpleTcpClient Connect(string hostNameOrIpAddress, int port); | ||
SimpleTcpClient Disconnect(); | ||
TcpClient TcpClient { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as .NET's TcpClient is not interfaced, ISimpleTcpClient won't help to mock Connected property.
I raised this PR a while ago due to not being able to mock the SimpleTcpClient (and thus making testing more difficult). At the time I worked around this issue (rather than waiting for the merge). As of right now I am no longer using the SimpleTcpClient and given the time taken to get this PR merged, I'm assuming it's not that much use to anyone else, so could probably be closed? |
Fixes #10