Add methods from BaseClient to ISftpClient

[benjaminoerskov]

Im still having a hard time mocking some of the methods in the SftpClient.
I came across the method Connect() which does not exist in the Interface, but comes from the BaseClient.
Would it be possible to add such methods to the ISftpClient interface? Or how would you mock them?

[DavidErben]

I am having the same issue! How are we supposed to do unit tests if we cannot mock essential methods like Connect() or Disconnect()? Since these methods are not virtual, I do not know any solution on how to mock them. If there is any, I would appreciate any help!

[benjaminoerskov]

@DavidErben I ended up writing my own wrapper. Quite simple:

[DavidErben]

@DavidErben I ended up writing my own wrapper.

This is probably the only solution, but I tried to avoid it since I do not want to have stuff inside my code base which is only needed for testing. 😄

[polly-utopi]

@DavidErben I ended up writing my own wrapper.

This is probably the only solution, but I tried to avoid it since I do not want to have stuff inside my code base which is only needed for testing. 😄

Also trying to avoid this 😅 Is there a reason it hasn't already been implemented?

[CasperWSchmidt]

@utopi-polly I guess lack of time from the maintainer(s)

[drieseng]

I'm sure guys, but I've spending way to much time working for my paid job lately.
Not adding that method was a serious oversight. We'll have to check if other methods or properties are M.I.A., but I'm sure there are (such as ConnectAsync(), Disconnect()).

[karpikpl]

Hey @benjaminoerskov ,
Shouldn't your wrapper implement IDisposable and Dispose underlying client?

[benjaminoerskov]

Thank you @karpikpl, I have updated my wrapper now :)

[CasperWSchmidt]

@drieseng Any news on this? Do you want a hand with it?

[CasperWSchmidt]

I just went through the BaseClient code and noticed the below public properties, events and methods that would need to be part of an interface. I suggest having a new interface IBaseClient (or similar) that inherits from IDisposable and allows ISftpClient etc. to inherit from it:

public interface IBaseClient : IDisposable
{
    bool IsConnected { get; }
    TimeSpan KeepAliveInterval { get; set; }
    ConnectionInfo ConnectionInfo { get; }
    event EventHandler<ExceptionEventArgs> ErrorOccurred;
    event EventHandler<HostKeyEventArgs> HostKeyReceived;
    void Connect();
    void Disconnect();
}

So ISftpClient interface is changed to: public interface ISftpClient: IBaseClient to make sure all public members of current BaseClient implementation gets accessible via the interfaces for each client (like SftpClient) extending it.

For now I'm making my own interface ISftpService that extends ISftpClient with the missing methods that I need (Connect, Disconnect and IsConnected) and an implementation wrapping the actual SftpClient like @benjaminoerskov and myself has suggested earlier.

[CasperWSchmidt]

On another note, the methods returning SftpFile (like ListDirectory and Get) should be modified to return ISftpFile instead to allow unit testing

[Owen-Krueger]

Thanks for the great suggestion on the IBaseClient, @CasperWSchmidt!

This issue should theoretically be fixed in #975.

[benjaminoerskov]

@Owen-Krueger Any idea when this will be released? I can see the last release was on May 29, 2022. I would like to start using this asap.

[Owen-Krueger]

That would be up to the owner of this solution, and I'm sure they are extremely busy. If there's anything I can do to help get this released, I can sure do that. I appreciate that you're excited for it, though 😄

[drieseng]

Fixed by @Owen-Krueger in #975.
Thanks!