feat(devbox): added devbox.shell(shellName) command and stateful shell class to SDK #696
Conversation
| >>> if result.success: | ||
| ... print("Task completed successfully!") | ||
| """ | ||
| # Ensure shell_name is set and cannot be overridden by user params |
There was a problem hiding this comment.
is it worth trying to make sure that the shell_name parameter is not exposed to the user at all? would probably be a hassle with types
There was a problem hiding this comment.
we want them to name their shells but we don't need to require it.
|
|
||
| # Verify we're in the new directory | ||
| pwd_result = await shell.exec("pwd") | ||
| pwd = (await pwd_result.stdout()).strip() |
There was a problem hiding this comment.
is stripping necessary? we already remove trailing newlines when calling .stdout()
| ) | ||
|
|
||
|
|
||
| class AsyncNamedShell: |
There was a problem hiding this comment.
should we provide a way to close the stateful shell? how do we handle how long a stateful shell remains alive on the backend?
There was a problem hiding this comment.
also since we're providing this as a first class object, it might make sense to provide a per-shell command history as a convenience method. what do you think?
There was a problem hiding this comment.
sounds good, future improvements
| duration = end_time - start_time | ||
| assert duration >= 2.9 # Allow some margin for overhead | ||
|
|
||
| # Verify all commands executed in order |
There was a problem hiding this comment.
not sure how this verifies commands executed in order
There was a problem hiding this comment.
the back end does it
jrvb-rl
left a comment
jrvb-rl
left a comment
There was a problem hiding this comment.
Looks very nice!!
A couple of suggestions on the tests, but those could easily be TODOs for later as well; minor cleanups.
|
|
||
| @pytest.mark.timeout(TWO_MINUTE_TIMEOUT) | ||
| async def test_shell_exec_sequential(self, devbox: AsyncDevbox) -> None: | ||
| """Test sequential execution (queuing).""" |
There was a problem hiding this comment.
Since this uses 'await shell.exec' throughouout, it isn't testing queueing; commands are not submitted before the previous one finishes
| """Test sequential async execution with queuing.""" | ||
| shell = devbox.shell("test-shell-8") | ||
|
|
||
| # Start multiple async commands - they should queue and execute sequentially |
There was a problem hiding this comment.
Let's also check that the time >= 3s for this test? That seems like a good idea
| result = await shell.exec( | ||
| 'echo "line1" && echo "line2" && echo "line3"', stdout=lambda line: stdout_lines.append(line) | ||
| ) | ||
|
|
||
| assert result.success is True | ||
| assert result.exit_code == 0 | ||
| assert len(stdout_lines) > 0 | ||
| stdout_combined = "".join(stdout_lines) | ||
| assert "line1" in stdout_combined | ||
| assert "line2" in stdout_combined | ||
| assert "line3" in stdout_combined | ||
| # Verify streaming captured same data as result | ||
| assert stdout_combined == await result.stdout() |
There was a problem hiding this comment.
At present, this isn't necessarily checking the queuing and callback behavior, since the callback might only be executed once for the whole output (it should be all available at the same time). If you add a sleep 1 or something in between, presumably this will ensure that you get 3 separate responses, and we could then check for those as separate things in stdout_lines w/o combining.
| stdout_combined = "".join(stdout_lines) | ||
| assert "async-line1" in stdout_combined |
There was a problem hiding this comment.
would be good to verify that len(stdout_lines) == 2 and the contents of each one
| # Test that additional params (like working_dir) are passed through correctly | ||
| # Note: shell_name should override any shell_name in params | ||
| result = await shell.exec("pwd", working_dir="/tmp") |
There was a problem hiding this comment.
q: does adding working_dir= to a persistent shell exec() function change the working dir for subsequent commands or not? Would be good to add a second "pwd" that verifies the documented behavior here, whatever it is.
| assert stderr_combined == await result.stderr() | ||
|
|
||
| @pytest.mark.timeout(TWO_MINUTE_TIMEOUT) | ||
| async def test_shell_exec_async_with_both_streams(self, devbox: AsyncDevbox) -> None: |
There was a problem hiding this comment.
Arguably we could just have sync/async versions of this test and then get rid of the separate stdout/stderr variants, just to simplify things a bit.
| shell = devbox.shell("test-shell-9") | ||
| stdout_lines: list[str] = [] | ||
|
|
||
| result = shell.exec( |
There was a problem hiding this comment.
same comment as the async case; better to have sleep in here, check the length of stdout_lines and the contents of each line separately
No description provided.