-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add tests for SftpFileStream.Seek(...) #9
Conversation
@daviburg, some of the tests will fail because the integration tests project consumes version 2020.0.1 of SSH.NET. See sshnet/SSH.NET#1048 for a discussion (monologue rather |
fs.Write(writeBuffer, 0, writeBuffer.Length); | ||
} | ||
|
||
using (var fs = client.OpenRead(remoteFile)) |
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.
This validation code is repeated several times. Should it be moved to a parametized helper method?
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 prefer to keep this as is to avoid having to navigate too much between methods in order to find out what is actually being tested.
OK?
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'm not too fond of how verbose and repetitive the current approach is, but it is not a showstopper.
@daviburg, thanks for taking the time to review these changes. Let me know if your approval still stands after my latest commit. |
Yes, I review the second commit and it looks good. |
Add tests for
SftpFileStream.Seek(...)
to cover sshnet/SSH.NET#910.