test(ssh): improve SSH proxy test coverage#1628
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1628 +/- ##
==========================================
+ Coverage 85.75% 90.30% +4.55%
==========================================
Files 84 84
Lines 8109 8109
Branches 1376 1466 +90
==========================================
+ Hits 6954 7323 +369
+ Misses 1126 757 -369
Partials 29 29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fabiovincenzi
changed the title
…git-proxy into ssh-test-coverage
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
Excellent work on getting our coverage back to standard! 🎉
Just a few minor comments/suggestions from me. I'd like to merge this ASAP for the coverage increase, despite conflicts with the Vitest 4 upgrade PR #1564. I'm happy to deal with failing tests later 🙂
| // Both call validateSSHPrerequisites | ||
| vi.mocked(validateSSHPrerequisites).mockClear(); | ||
|
|
||
| await fetchGitHubCapabilities( |
There was a problem hiding this comment.
If fetchGitHubCapabilities defaults to true and then we assert that validateSSHPrerequisites was called, isn't that incompatible with the test's description?
|
|
||
| function mockChannelModule(channelImpl?: (...args: unknown[]) => unknown) { | ||
| const mockChannel = { on: vi.fn(), write: vi.fn(), end: vi.fn() }; | ||
| getChannelModuleSpy = vi.spyOn(sshInternals, 'getChannelModule').mockReturnValue({ |
There was a problem hiding this comment.
These mockReturnValue usages will likely fail after upgrading to Vitest 4 (underway in #1564).
If we merge this in first, and I can fix up any failing tests in that PR, otherwise you can fix those by passing in a function that returns an object, rather than the object itself.
| } as any); | ||
|
|
||
| const debugServer = new SSHServer(); | ||
| expect(debugServer).toBeDefined(); |
There was a problem hiding this comment.
We may want to check some other properties to make sure the server actually did start in debug mode 👍🏼
| const ssh2 = await import('ssh2'); | ||
| const Client = ssh2.Client as any; | ||
|
|
||
| const chunk1 = Buffer.from('0010chunk one\n'); |
There was a problem hiding this comment.
I think these pkt-line definitions might be wrong - though it's not clear why the test passes regardless:
0010 means the payload has 16 bytes, but the actual bytes in the payload is 14 (4 + 9 + 1):
"0010" = 4 bytes
"chunk one" = 9 bytes
"\n" = 1 byte
Same for the other chunk which excludes the flush.
| const chunk1 = Buffer.from('0010chunk one\n'); | |
| const chunk1 = Buffer.from('000echunk one\n'); |
| const Client = ssh2.Client as any; | ||
|
|
||
| const chunk1 = Buffer.from('0010chunk one\n'); | ||
| const chunk2 = Buffer.from('0010chunk two\n0000'); |
There was a problem hiding this comment.
| const chunk2 = Buffer.from('0010chunk two\n0000'); | |
| const chunk2 = Buffer.from('000echunk two\n0000'); |
| const Client = ssh2.Client as any; | ||
|
|
||
| // Build a valid pkt-line payload with flush packet | ||
| // "0012hello world\n0000" |
There was a problem hiding this comment.
This comment may also be inaccurate since "hello world\n" has 12 bytes + 2 + 2 = 16 bytes, or 0010 in hexadecimal
| // "0012hello world\n0000" | |
| // "0010hello world\n0000" |
2 participants
Description
Add tests to improve SSH proxy coverage, focusing on the four modules with the most gaps:
Related Issue
Resolves #1586
Checklist
General
Documentation
Configuration
config.schema.json) was modified (N/A — no config changes)Tests
npm test)npm run lintandnpm run format:check)npm run check-types)