Skip to content

Add interface to SshCommand#1775

Closed
MartinHvolgaardHansen wants to merge 3 commits intosshnet:developfrom
MartinHvolgaardHansen:refactor/create-sshcommand-interface
Closed

Add interface to SshCommand#1775
MartinHvolgaardHansen wants to merge 3 commits intosshnet:developfrom
MartinHvolgaardHansen:refactor/create-sshcommand-interface

Conversation

@MartinHvolgaardHansen
Copy link
Copy Markdown

This update adds an interface to SshCommand to enable easy mocking in dependent tests.

A quick overview of the changes:

  1. Add the ISshCommand interface.
  2. Move breaking changes to implementations to the V2 namespace to ensure backwards compatibility.
  3. Change existing implementations to be thin wrappers around changed implementations.
  4. Update and run tests associated with the changes (except the Docker integration tests).
  5. Update examples in doc files to reflect these changes.

There are alternatives to achieve testability, but for SSH.NET to be truly testable in future projects, this interface is needed.

Did I miss anything? Let me know, and I'll make sure to amend it.

Thanks for reviewing and your time, and a big thanks to the authors and contributors.

@Rob-Hague
Copy link
Copy Markdown
Collaborator

I am sorry, but this change won't be taken (due to the extensive duplication and therefore additional maintenance). This topic has already been discussed in e.g. #1508, #1725.

In the future, I would recommend creating an issue or commenting on an existing one to discuss this kind of change before putting in the work (just to save you time)

@Rob-Hague Rob-Hague closed this Apr 1, 2026
@MartinHvolgaardHansen
Copy link
Copy Markdown
Author

Hi Rob,

I did review #1508 and, while solving the underlying problem, his changes are breaking whereas mine is not.
Now, I also read #1725, but regardless of how many wrappers we add, there's no real way of testing the grabbing of 'command.Result' without an interface to mock.

I made sure that there's next to no 'duplication', as I've mentioned in the describtion, so I'm unsure of what you mean by that?

Both of the above are the concerns you've previously mentioned, which this change effectively resolves - What would you propose otherwise?

Thanks for your consideration.

Br,
Martin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants