Add TestServer.SSH#54
Merged
Merged
Conversation
richard-ash
force-pushed
the
richard/add-ssh
branch
3 times, most recently
from
1f89cfb to
0fe9f1f
Compare
richard-ash
force-pushed
the
richard/add-ssh
branch
from
0fe9f1f to
3e767f0
Compare
danschultzer
reviewed
Apr 1, 2026
richard-ash
force-pushed
the
richard/add-ssh
branch
8 times, most recently
from
99e376d to
8798c67
Compare
danschultzer
reviewed
Apr 3, 2026
richard-ash
force-pushed
the
richard/add-ssh
branch
3 times, most recently
from
ab429b1 to
089dd44
Compare
richard-ash
force-pushed
the
richard/refactor-http-websocket
branch
from
48b96e7 to
65e2e94
Compare
richard-ash
force-pushed
the
richard/add-ssh
branch
2 times, most recently
from
eb5647e to
9f345f8
Compare
danschultzer
reviewed
Apr 6, 2026
richard-ash
force-pushed
the
richard/add-ssh
branch
3 times, most recently
from
a7c2b93 to
146a4c4
Compare
richard-ash
force-pushed
the
richard/refactor-http-websocket
branch
from
65e2e94 to
9be9f90
Compare
richard-ash
force-pushed
the
richard/add-ssh
branch
3 times, most recently
from
cc36c29 to
78b31c6
Compare
richard-ash
force-pushed
the
richard/add-ssh
branch
3 times, most recently
from
354f99d to
f5d3b6e
Compare
richard-ash
force-pushed
the
richard/add-ssh
branch
from
f5d3b6e to
3e7f7d4
Compare
richard-ash
force-pushed
the
richard/add-ssh
branch
2 times, most recently
from
c9c243a to
848163a
Compare
Comment on lines
+27
to
+28
| "#{TestServer.format_instance(TestServer.SSH, state.instance)} received an unexpected SSH channel up message for channel ID #{channel_id} on connection #{inspect(connection)}." | ||
| |> append_formatted_channels(state.instance) |
Owner
There was a problem hiding this comment.
This goes all the way down, but a single pipe shouldn't really be used.
richard-ash
force-pushed
the
richard/refactor-http-websocket
branch
3 times, most recently
from
20aee7b to
7b41e97
Compare
An error occurred while trying to automatically change base from
richard/refactor-http-websocket
to
main
April 28, 2026 18:26
richard-ash
force-pushed
the
richard/add-ssh
branch
from
848163a to
aac5462
Compare
|
|
||
| alias TestServer.SSH.Instance | ||
|
|
||
| defstruct [:instance, :channel, :custom] |
Owner
There was a problem hiding this comment.
The :custom doesn't feel right, is there a better name?
Comment on lines
+162
to
+199
| defp append_formatted_channels(message, instance) do | ||
| channels = Enum.split_with(Instance.channels(instance), &is_nil(&1.channel_id)) | ||
|
|
||
| """ | ||
| #{message} | ||
|
|
||
| #{format_channels(channels)} | ||
| """ | ||
| end | ||
|
|
||
| defp format_channels({[], []}) do | ||
| "No available channels." | ||
| end | ||
|
|
||
| defp format_channels({[], used}) do | ||
| """ | ||
| No available channels. The following channels have been claimed: | ||
|
|
||
| #{Instance.format_channels(used)} | ||
| """ | ||
| end | ||
|
|
||
| defp format_channels({available, _used}) do | ||
| """ | ||
| Available channels: | ||
|
|
||
| #{Instance.format_channels(available)} | ||
| """ | ||
| end | ||
|
|
||
| defp send_error(connection, channel_id, state, {exception, stacktrace}) do | ||
| Instance.report_error(state.instance, {exception, stacktrace}) | ||
|
|
||
| error_message = Exception.format(:error, exception, stacktrace) | ||
| :ssh_connection.send(connection, channel_id, error_message) | ||
|
|
||
| {state, [exit_status: 1]} | ||
| end |
Owner
There was a problem hiding this comment.
This should be moved up above handle_ssh_msg as it's the first place it is referenced.
Comment on lines
+99
to
+121
| case result do | ||
| {:ok, {:noreply, custom}} -> | ||
| {%{state | custom: custom}, []} | ||
|
|
||
| {:ok, {:reply, {data, opts}, custom}} -> | ||
| :ssh_connection.send(connection, channel_id, data) | ||
|
|
||
| stderr = Keyword.get(opts, :stderr) | ||
| stderr && :ssh_connection.send(connection, channel_id, 1, to_string(stderr)) | ||
|
|
||
| {%{state | custom: custom}, opts} | ||
|
|
||
| {:error, :not_found} -> | ||
| message = | ||
| "#{TestServer.format_instance(TestServer.SSH, state.instance)} received an unexpected SSH message of type #{type}:" | ||
| |> append_formatted_frame(frame) | ||
| |> append_formatted_handlers(state.instance) | ||
|
|
||
| send_error(connection, channel_id, state, {RuntimeError.exception(message), []}) | ||
|
|
||
| {:error, {exception, stacktrace}} -> | ||
| send_error(connection, channel_id, state, {exception, stacktrace}) | ||
| end |
Owner
There was a problem hiding this comment.
Does it look better if we make this case just the function header? Not sure if a case here is easier to read than just having three functions.
| @spec dispatch(pid(), {:channel_up, non_neg_integer(), pid()}) :: | ||
| {:ok, {reference(), keyword(), TestServer.stacktrace()}} | ||
| | {:error, :not_found} | ||
| | {:error, {:already_taken, TestServer.SSH.channel()}} |
Owner
There was a problem hiding this comment.
Is :already_taken the best describer? I wonder if there's something more OTP/Elixir like, but maybe it's fine as per the not_found above. But I also think there may be a bit of mixed language of used vs taken?
| defp ensure_function!(fun) when is_function(fun), do: :ok | ||
| defp ensure_function!(fun), do: raise(BadFunctionError, term: fun) | ||
|
|
||
| @spec dispatch(pid(), {:channel_up, non_neg_integer(), pid()}) :: |
Owner
There was a problem hiding this comment.
Instead of non_neg_integer isn't this just an ssh channel id? may even exist in the :ssh module as a type.
| GenServer.call(instance, {:register, {:channel, {options, stacktrace}}}) | ||
| end | ||
|
|
||
| @spec register(pid(), {:handle, {reference(), keyword(), TestServer.stacktrace()}}) :: |
Owner
There was a problem hiding this comment.
The reference is the is actually the channel right? We can use the type from SSH?
| GenServer.call(instance, {:dispatch, {:channel_up, channel_id, connection}}) | ||
| end | ||
|
|
||
| @spec dispatch(pid(), {:handle, reference(), reference(), tuple(), map()}) :: |
Owner
There was a problem hiding this comment.
I think it can be SSH.channel(), and maybe there is types for the next two in :ssh?.
| %{ | ||
| options: options, | ||
| channels: [], | ||
| connections: %{}, |
richard-ash
force-pushed
the
richard/add-ssh
branch
2 times, most recently
from
4a04923 to
d1ff0d4
Compare
danschultzer
force-pushed
the
richard/add-ssh
branch
6 times, most recently
from
6d849d0 to
dc7e815
Compare
Co-Authored-By: Dan Schultzer <1254724+danschultzer@users.noreply.github.com> # Conflicts: # lib/test_server/http.ex
danschultzer
force-pushed
the
richard/add-ssh
branch
from
dc7e815 to
c76044f
Compare
Owner
|
🤩 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.