hotfix(cli): replace raw subprocess tracebacks with styled error messages and fix restart race condition #96
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
@@ -18,29 +18,32 @@ def kill():
|
||||
sys.stderr.write(result.stderr.decode())
|
||||
|
||||
|
||||
def start_instance(no_daemon: bool = False) -> None:
|
||||
result = subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), capture_output=True)
|
||||
stdout = result.stdout.decode().strip()
|
||||
|
zach marked this conversation as resolved
Outdated
|
||||
if stdout:
|
||||
if "already running" in stdout.lower():
|
||||
raise click.ClickException(stdout)
|
||||
if result.returncode != 0:
|
||||
stderr = result.stderr.decode().strip()
|
||||
raise click.ClickException(stderr)
|
||||
|
||||
|
||||
@app.command()
|
||||
def start(no_daemon: bool = False):
|
||||
check = subprocess.run(args + ["ipc"] + ["show"], capture_output=True)
|
||||
if check.returncode == 0:
|
||||
raise click.ClickException("An instance of this configuration is already running.")
|
||||
result = subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), capture_output=True)
|
||||
if result.returncode != 0:
|
||||
raise click.ClickException(result.stderr.decode().strip())
|
||||
sys.stderr.write(result.stderr.decode())
|
||||
start_instance(no_daemon)
|
||||
|
||||
|
AramJonghu marked this conversation as resolved
Outdated
zach
commented
This seems ugly. Personally think we should just set a sleep timer of ~5 seconds, then run code. This seems ugly. Personally think we should just set a sleep timer of ~5 seconds, then run code.
AramJonghu
commented
Is that not an messier implementation? A user would want a restart to be quick (quicker than running the kill and start command separately). 2.5 or higher stock sleep timer would make little sense. time.monotonic() could be used to hide the ugliness, but same logic/idea. Is that not an messier implementation? A user would want a restart to be quick (quicker than running the kill and start command separately). 2.5 or higher stock sleep timer would make little sense.
time.monotonic() could be used to hide the ugliness, but same logic/idea.
zach
commented
Running a loop would keep a CPU thread busy until it's able to launch the shell, while a sleep timer wouldn't. I don't really see much benefit in looping upwards of 50 times just to make sure we can run the start command exactly when it's available. Running a loop would keep a CPU thread busy until it's able to launch the shell, while a sleep timer wouldn't. I don't really see much benefit in looping upwards of 50 times just to make sure we can run the start command exactly when it's available.
AramJonghu
commented
Is that not okay for a rare command as restart? We could change the loop to check every 250ms instead, so it is 10 checks instead of 50. Is that not okay for a rare command as restart?
We could change the loop to check every 250ms instead, so it is 10 checks instead of 50.
zach
commented
That's a good middle ground I think, agreed. That's a good middle ground I think, agreed.
AramJonghu
commented
Changed it. Changed it.
|
||||
|
||||
@app.command()
|
||||
def restart(no_daemon: bool = False):
|
||||
subprocess.run(args + ["kill"])
|
||||
for _ in range(50):
|
||||
subprocess.run(args + ["kill"], capture_output=True)
|
||||
deadline = time.monotonic() + 2.5
|
||||
|
zach marked this conversation as resolved
Outdated
zach
commented
Here we can just call Here we can just call `start()`, right? Not sure if `Typer` allows calling command functions.
AramJonghu
commented
It was to have separate tests. Simpler in my eyes (at least at time of writing). I can simplify it by having a It was to have separate tests. Simpler in my eyes (at least at time of writing). I can simplify it by having a `start_instance()` that start uses outright, and is reused by restart. This could satisfy testing and reuse of code, and perhaps future code changes.
|
||||
while time.monotonic() < deadline:
|
||||
result = subprocess.run(args + ["kill"], capture_output=True)
|
||||
if result.returncode == 255:
|
||||
break
|
||||
time.sleep(0.05)
|
||||
result = subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), capture_output=True)
|
||||
if result.returncode != 0:
|
||||
raise click.ClickException(result.stderr.decode().strip())
|
||||
sys.stderr.write(result.stderr.decode())
|
||||
start_instance(no_daemon=no_daemon)
|
||||
|
||||
|
||||
@app.command()
|
||||
|
||||
+21
-26
@@ -34,35 +34,30 @@ class TestKill:
|
||||
class TestStart:
|
||||
@patch("zshell.subcommands.shell.subprocess.run")
|
||||
def test_start_default_daemon(self, mock_run):
|
||||
mock_run.side_effect = [
|
||||
CompletedProcess([], 1, b"", b""), # ipc show → no instance
|
||||
CompletedProcess([], 0, b"", b"Launching config\n"), # launch ok
|
||||
]
|
||||
mock_run.return_value = CompletedProcess([], 0, b"", b"Launching config\n")
|
||||
invoke("start")
|
||||
assert mock_run.call_args_list == [
|
||||
call(["qs", "-c", "zshell", "ipc", "show"], capture_output=True),
|
||||
call(["qs", "-c", "zshell", "-n", "-d"], capture_output=True),
|
||||
]
|
||||
mock_run.assert_called_once_with(["qs", "-c", "zshell", "-n", "-d"], capture_output=True)
|
||||
|
||||
@patch("zshell.subcommands.shell.subprocess.run")
|
||||
def test_start_no_daemon(self, mock_run):
|
||||
mock_run.side_effect = [
|
||||
CompletedProcess([], 1, b"", b""),
|
||||
CompletedProcess([], 0, b"", b"Launching config\n"),
|
||||
]
|
||||
mock_run.return_value = CompletedProcess([], 0, b"", b"Launching config\n")
|
||||
invoke("start", "--no-daemon")
|
||||
assert mock_run.call_args_list == [
|
||||
call(["qs", "-c", "zshell", "ipc", "show"], capture_output=True),
|
||||
call(["qs", "-c", "zshell", "-n"], capture_output=True),
|
||||
]
|
||||
mock_run.assert_called_once_with(["qs", "-c", "zshell", "-n"], capture_output=True)
|
||||
|
||||
@patch("zshell.subcommands.shell.subprocess.run")
|
||||
def test_start_already_running_errors(self, mock_run):
|
||||
mock_run.return_value = CompletedProcess([], 0, b"", b"target visibilities\n")
|
||||
mock_run.return_value = CompletedProcess([], 0, b"An instance of this configuration is already running.\n", b"")
|
||||
result = runner.invoke(app, ["start"])
|
||||
assert result.exit_code != 0
|
||||
assert "already running" in result.output
|
||||
|
||||
@patch("zshell.subcommands.shell.subprocess.run")
|
||||
def test_start_other_failure_errors(self, mock_run):
|
||||
mock_run.return_value = CompletedProcess([], 1, b"", b"Config error\n")
|
||||
result = runner.invoke(app, ["start"])
|
||||
assert result.exit_code != 0
|
||||
assert "Config error" in result.output
|
||||
|
||||
|
||||
class TestShow:
|
||||
@patch("zshell.subcommands.shell.subprocess.run")
|
||||
@@ -106,30 +101,30 @@ class TestCall:
|
||||
|
||||
|
||||
class TestRestart:
|
||||
@patch("zshell.subcommands.shell.start_instance")
|
||||
@patch("zshell.subcommands.shell.subprocess.run")
|
||||
def test_restart_kills_then_starts(self, mock_run):
|
||||
def test_restart_kills_then_starts(self, mock_run, mock_start):
|
||||
mock_run.side_effect = [
|
||||
CompletedProcess([], 0, b"", b"Killed abc\n"), # first kill (no capture)
|
||||
CompletedProcess([], 0, b"", b"Killed abc\n"), # first kill (captured)
|
||||
CompletedProcess([], 255, b"", b""), # poll → no instance
|
||||
CompletedProcess([], 0, b"", b"Launching config\n"), # launch ok
|
||||
]
|
||||
invoke("restart")
|
||||
assert mock_run.call_args_list == [
|
||||
call(["qs", "-c", "zshell", "kill"]), # no capture_output
|
||||
call(["qs", "-c", "zshell", "kill"], capture_output=True),
|
||||
call(["qs", "-c", "zshell", "-n", "-d"], capture_output=True),
|
||||
call(["qs", "-c", "zshell", "kill"], capture_output=True),
|
||||
]
|
||||
mock_start.assert_called_once_with(no_daemon=False)
|
||||
|
||||
@patch("zshell.subcommands.shell.start_instance")
|
||||
@patch("zshell.subcommands.shell.subprocess.run")
|
||||
def test_restart_no_daemon(self, mock_run):
|
||||
def test_restart_no_daemon(self, mock_run, mock_start):
|
||||
mock_run.side_effect = [
|
||||
CompletedProcess([], 0, b"", b"Killed abc\n"),
|
||||
CompletedProcess([], 255, b"", b""),
|
||||
CompletedProcess([], 0, b"", b"Launching config\n"),
|
||||
]
|
||||
invoke("restart", "--no-daemon")
|
||||
assert mock_run.call_args_list == [
|
||||
call(["qs", "-c", "zshell", "kill"]),
|
||||
call(["qs", "-c", "zshell", "kill"], capture_output=True),
|
||||
call(["qs", "-c", "zshell", "-n"], capture_output=True),
|
||||
call(["qs", "-c", "zshell", "kill"], capture_output=True),
|
||||
]
|
||||
mock_start.assert_called_once_with(no_daemon=True)
|
||||
|
||||
Reference in New Issue
Block a user
The start command should not need to run
qs -c zshell ipc call, IPC calls are used to trigger states and functions within the QML code. The-nflag already checks if there's a running instance of the shell.Was not aware of this. Remaining functions use ipc, which is why I did not bat an eye. Resolving now.
Change occurred since I wanted an error block such as in the screenshot. But with that info, ipc is indeed not needed in the code.