chore(transport/ssh): Increase test coverage (+bug fixes) #109

2025-12-30T04:45:49Z

nemith commented

2025-12-30 04:45:49 +00:00

(Migrated from github.com)

The original transport/ssh tests were limited. This expands the test coverage to more coverage. In the process a couple bugs were found and fixed.

When calling newTransport fails we are responsible for closing the connection
Fixed extra call to Close()

❯ go test ./transport/ssh -cover 
ok      nemith.io/netconf/transport/ssh 0.683s  coverage: 82.2% of statements

The original transport/ssh tests were limited. This expands the test coverage to more coverage. In the process a couple bugs were found and fixed. - When calling newTransport fails we are responsible for closing the connection - Fixed extra call to Close() ``` ❯ go test ./transport/ssh -cover ok nemith.io/netconf/transport/ssh 0.683s coverage: 82.2% of statements ```

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-12-30 04:48:23 +00:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull request overview

This PR significantly expands test coverage for the SSH transport module from limited coverage to 82.2%, while fixing two critical bugs discovered during the testing process:

Bug fix: When newTransport fails, the SSH client is now properly closed to prevent resource leaks
Bug fix: Removed duplicate Close() call in the error path

Key Changes

Refactored test infrastructure with a reusable testServer helper using dynamically generated ED25519 keys
Added comprehensive error scenario tests covering network failures, authentication failures, context cancellation, and subsystem rejection
Fixed resource leak when newTransport fails by ensuring the client connection is closed
Improved error handling using errors.Join to properly aggregate multiple close errors

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
transport/ssh/ssh.go	Fixed client connection leak on transport creation failure, improved error aggregation with `errors.Join`, and updated context handling comments
transport/ssh/ssh_test.go	Complete test suite rewrite with new `testServer` helper, added 5 new test cases covering error scenarios, and improved test structure for better maintainability

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview This PR significantly expands test coverage for the SSH transport module from limited coverage to 82.2%, while fixing two critical bugs discovered during the testing process: - Bug fix: When `newTransport` fails, the SSH client is now properly closed to prevent resource leaks - Bug fix: Removed duplicate `Close()` call in the error path ### Key Changes - Refactored test infrastructure with a reusable `testServer` helper using dynamically generated ED25519 keys - Added comprehensive error scenario tests covering network failures, authentication failures, context cancellation, and subsystem rejection - Fixed resource leak when `newTransport` fails by ensuring the client connection is closed - Improved error handling using `errors.Join` to properly aggregate multiple close errors ### Reviewed changes Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments. | File | Description | | ---- | ----------- | | transport/ssh/ssh.go | Fixed client connection leak on transport creation failure, improved error aggregation with `errors.Join`, and updated context handling comments | | transport/ssh/ssh_test.go | Complete test suite rewrite with new `testServer` helper, added 5 new test cases covering error scenarios, and improved test structure for better maintainability | --- 💡 <a href="/nemith/netconf/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

transport/ssh/ssh.go

					
				@ -22,7 +23,7 @@ type Transport struct {

					// set to true if the transport is managing the underlying ssh connection

The field name "mangedConn" contains a typo - it should be "managedConn" with an 'a'. This typo appears in the field declaration and is consistently used throughout the file.

transport/ssh/ssh.go

					
				@ -105,10 +112,10 @@ func newTransport(client *ssh.Client, managed bool) (*Transport, error) {

					}

The field name "mangedConn" contains a typo - it should be "managedConn" with an 'a'.

transport/ssh/ssh.go

					
				@ -118,22 +125,23 @@ func newTransport(client *ssh.Client, managed bool) (*Transport, error) {

				// with Dial then then underlying ssh.Client is closed as well.  If not only

The word "connnection" has an extra 'n' - it should be "connection".

			return errors.Join(retErr, fmt.Errorf("failed to close ssh connection: %w", err))

The word "connnection" has an extra 'n' - it should be "connection". ```suggestion return errors.Join(retErr, fmt.Errorf("failed to close ssh connection: %w", err)) ```

The field name "mangedConn" contains a typo - it should be "managedConn" with an 'a'.

transport/ssh/ssh_test.go

					
				@ -134,2 +132,3 @@

					r, err := tr.MsgReader()

					assert.NoError(t, err)

					require.NoError(t, err)

					greeting, _ := io.ReadAll(r)

Error handling is incomplete - the returned errors from io.WriteString and io.ReadAll are being ignored. While this may be acceptable in a test handler, it's better to check these errors or explicitly ignore them with a comment explaining why.

Error handling is incomplete - the returned errors from tr.MsgReader(), io.ReadAll, tr.MsgWriter(), io.WriteString, w.Close(), and tr.Close() are being ignored. While this may be acceptable in a test, it's better to check these errors or use require.NoError/assert.NoError to catch unexpected failures.

The return value from io.ReadAll is being ignored. While this may be intentional in this test context, it's better to explicitly check the error or add a comment explaining why it's safe to ignore.

		_, err := io.ReadAll(ch)
		if err != nil {
			return err
		}

The return value from io.ReadAll is being ignored. While this may be intentional in this test context, it's better to explicitly check the error or add a comment explaining why it's safe to ignore. ```suggestion _, err := io.ReadAll(ch) if err != nil { return err } ```

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

nemith/netconf!109

				`@ -22,7 +23,7 @@ type Transport struct {`
				`// set to true if the transport is managing the underlying ssh connection`

				`@ -105,10 +112,10 @@ func newTransport(client ssh.Client, managed bool) (Transport, error) {`
				`}`

				`@ -118,22 +125,23 @@ func newTransport(client ssh.Client, managed bool) (Transport, error) {`
				`// with Dial then then underlying ssh.Client is closed as well. If not only`

Rows
Columns