Issue #193 Adding ISftpClient interface to SftpClient

[ericbrumfield]

Issue #193 Adding ISftpClient interface to SftpClient for mocking and testing purposes.

[ericbrumfield]

Hi @drieseng, if there's more I need to do to get the CI passing or if there are more changes to be made on this just let me know. Sorry if I missed anything obvious or if my PR is wrong. I don't participate on github enough and am trying to remedy that.

[jherbert]

@ericbrumfield Have you heard back on this PR? I'm in the same boat that I'm looking to mock the client for tests.

[ericbrumfield]

@jherbert I have not heard anything back regarding it, but am more than happy to revisit this PR if I need to.

For my needs at the time when I wanted this feature, I ended up writing a test that actually spun up an SFTP server instance and had the test do a complete end to end file upload. The library I used for that was sftp-lite and can be found here, that may help your test efforts until we can get this interface in for mocking. I'd prefer to mock but sftp-lite worked in a pinch.

[MartinsRui]

I would also like to mock the SftpClient for testing purposes.

[melih154]

I also need this, so trying to help:)

Seems like Build error reported on here needs to be fixed.

1. ISftpClient file reference is added to csproj, but I can't see the content of ISftpClient on the commit itself. Can you make sure that ISftpClient.cs implementation is within the commit(a new one)?

2.Below build error message shows that SftpFileStream.cs has reference to System.Diagnostics.Contacts, which is not correct, right? ISftpClient should be under at least Renci.SshNet namespace.

C:\projects\ssh-net\src\Renci.SshNet\Sftp\SftpFileStream.cs(6,26): error CS0234: The type or namespace name 'Contracts' does not exist in the namespace 'System.Diagnostics' (are you missing an assembly reference?) [C:\projects\ssh-net\src\Renci.SshNet.NET35\Renci.SshNet.NET35.csproj]

[ericbrumfield]

Thanks @melih154 , I'll take a look into it again the next chance I get, I'd like to get this in as well. Life has been pretty busy lately but I should hopefully be able to take another look soon.

[augustoproiete]

@drieseng and/or @olegkap any chance to look at this one? Happy to help to move this forward and get it released.

[lanegoolsby]

Having this would be immensely valuable. Otherwise we are stuck writing stubs or abstraction wrappers in order to write unit tests....

[mikejr83]

I've abstracted as much as I can in my current project and with our high unit test standards I'm failing our coverage tests because I'm having trouble mocking and getting around the client. Having this will allow me to get unit test coverage on an important service in my app.

[drieseng]

I'll take a look at it asap.
Thanks!

[abindal1986]

Hi.
Is there any progress on this. I'm also failing on the coverage tests and finding it difficult for the mocking to workaround it. Any help in regards to a workaround or a fix for this will greatly benefit the community.

Thanks.

[abindal1986]

Hi.
Is there any progress on this. I'm also failing on the coverage tests and finding it difficult for the mocking to workaround it. Any help in regards to a workaround or a fix for this will greatly benefit the community.

Thanks.

[drieseng]

I'm ok with adding this interface, but I'd like to keep the interface minimal.
For example, I do not want to include obsolete members.
I'd also prefer not to include BufferSize and OperationTimeout in this interface.
Would that block its usefulness for any of you?

[lanegoolsby]

We use BufferSize so we'd need it (or an alternative). Has BufferSize been deprecated?

[augustoproiete]

@drieseng It makes sense not to include obsolete members, but I'd argue that - for the purposes of mocking - any public members of the class should be included in the interface... No exceptions.

[schwede]

I just want to chime in that this would be a great addition to the library. As others have mentioned, I have used the same approach as writing my own wrappers for SftpClient for tests. Direct support from the library would make this easier/better.

[EdvardsenErik]

I'd also love this addition. I too am writing my own wrappers due to testing, this would make life simpler and easier for us.

[jayserdny]

I also really need this interface. It is too hard to test this library without an interface :/

[williamb-egov]

bump?

[jayserdny]

I ended up creating yet another wrapper for that library to expose an interface to the client.

[williamb-egov]

@drieseng Any advice on what to do to reconcile this bug at this point? Mocking this class would be a huge step towards testing.

[hansmelin]

My vote for this improvement. I need to mock the client in my tests too.

[drieseng]

@ericbrumfield Are you still interested in working with me to get this in? If so, please:

• rebase this PR on top of the develop branch.
• remove obsolete members from ISftpClient.

[ericbrumfield]

@drieseng Yes will do, I'll work to do an update to get it in.

[ericbrumfield]

Obsolete members should be gone from the new interface. Updated comments to match latest implementation side. Also, I believe I've rebased on top of upstream/develop correctly, my first rebase here, so let me know if anything else is needed.

[drieseng]

@ericbrumfield I'm afraid the rebase didn't work out (see https://github.com/sshnet/SSH.NET/pull/197/files). Do you want to take another stab at it, or are you ok with me just taking the interface from your PR ? (I'll still credit you for it, of course)

[ericbrumfield]

@drieseng Sorry about that, being my first rebase I'm not sure what I did wrong, but another update is up now with passing tests. All I really did to remedy was pulled upstream develop back to my local and pushed up again. File diff looks much better so I guess I did it right? If I'm doing something wrong, setup wrong or you need something else feel free to let me know.

[drieseng]

@ericbrumfield No need to say sorry at all. Thanks!

[caio-ody]

Hi, sorry for the silly question, but does the merge mean we'll be getting a new release, so we can update our nuget and use the new interface? Thanks :)

[ericbrumfield]

Hi, sorry for the silly question, but does the merge mean we'll be getting a new release, so we can update our nuget and use the new interface? Thanks :)

@caio-ody I don't want to speak for the project maintainers, but I assume eventually it would be part of a release where it was added as part of this milestone.

[drieseng]

@caio-ody Work and great weather have caused a delay. I'll try to release a new/last beta in the next week or so.

[ghost]

@drieseng Any updates on this? We're looking to use this interface for testing.