Hi Petr,
On 2026-10-10T12:27:54, Petr Hodina via B4 Relay
<[email protected]> wrote:
> test: button: add test for monitor subcommand
>
> Run 'button monitor 200' on sandbox and check that the output lists
> button1 and button2 and that the command returns 0 after the timeout.
>
> Signed-off-by: Petr Hodina <[email protected]>
>
> test/py/tests/test_button.py | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> diff --git a/test/py/tests/test_button.py b/test/py/tests/test_button.py
> @@ -37,3 +37,23 @@ def test_button_return_code(ubman):
> + response = ubman.run_command('button monitor 200; echo rc:$?')
> + assert('Button monitor' in response)
> + assert('button1' in response)
> + assert('button2' in response)
> + assert('rc:0' in response)
This only checks the labels, which 'button list' already covers, and
not the printed states. Please can you drive a3 high first, as
test_button_return_code() does, and check that button1 shows 'on'?
I also wonder whether this would be better as a C test in
test/dm/button.c, using console recording. You could then check the
exact lines, including the escape sequences, with ut_assert_nextline()
and similar. You could also cover the Ctrl-C path, which is new in v2
and has no test yet: call console_in_puts('\x03') and then run 'button
monitor 0'. At present a timeout of 0 has no test at all, and a bug
there would hang the run. A C test would also run faster than pytest.
What do you think?
> diff --git a/test/py/tests/test_button.py b/test/py/tests/test_button.py
> @@ -37,3 +37,23 @@ def test_button_return_code(ubman):
> + response = ubman.run_command('button monitor abc; echo rc:$?')
> + assert('Button monitor' not in response)
> + assert('rc:1' in response)
The CMD_RET_USAGE text includes 'button monitor [timeout_ms]', so this
check only passes because of the capital B. It would be clearer to
check that 'Usage' is in the response. With a C test you can check for
run_command() returning CMD_RET_USAGE instead of 0. Please can you
also add a case with trailing junk such as 'button monitor 200x',
since that is what the '*endp' check in patch 1 is for?
Regards,
Simon
