Hi Simon,
On Sat, Oct 31, 2026 at 4:02 PM Simon Glass <[email protected]> wrote:
>
> Hi Maxin,
>
> On 2026-10-29T17:34:37+11:00, Maxin John <[email protected]> wrote:
> > test: cmd: add tests for the binop command
> >
> > binop has had no tests so far. Add some, so that bugs like the heap
> > overrun are caught in CI.
> >
> > Each operation (xor, or, and) is run on hex strings, env vars or
> > memory, with the result going to an env var, memory or the console.
> >
> > The tests also check that:
> > - short inputs are zero-padded
> > - console output wraps after 16 bytes
> > - bad counts, operators, addresses or hex strings are rejected
> > without touching the destination
> >
> > There is also a test for the heap overrun.
> >
> > Signed-off-by: Maxin B. John <[email protected]>
> >
> > test/cmd/Makefile | 1 +
> > test/cmd/binop.c | 192
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 193 insertions(+)
>
> > diff --git a/test/cmd/binop.c b/test/cmd/binop.c
> > @@ -0,0 +1,192 @@
> > + /* Output wraps after 16 bytes */
> > + ut_assertok(run_command("binop xor 17
> > 000102030405060708090a0b0c0d0e0f10 00",
> > + 0));
> > + ut_assert_nextline("00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> > ");
> > + ut_assert_nextline("10 ");
> > + ut_assert_console_end();
>
> Please can you add a case with a count of exactly 16? With the current
> loop, i == 15 outputs a newline and the final puts('\n') outputs
> another, so the user sees a blank line after the data. If that is
> intended, a test should pin it down. Otherwise it is worth fixing in
> the binop clean-up patch, since this is the edge case most likely to
> go wrong.
Thanks for the review. I have addressed this in v3 of binop clean-up patch.
Also added a test for exactly 16 bytes to check the extra newline.
> > diff --git a/test/cmd/binop.c b/test/cmd/binop.c
> > @@ -0,0 +1,192 @@
> > +static int cmd_test_binop_invalid(struct unit_test_state *uts)
> > +{
> > + /* Failed operations must not change binop_out */
> > + ut_assertok(env_set("binop_out", "deadbeef"));
> > +
> > + /* Literal hex string: too long, odd length, not hex */
> > + ut_asserteq(1, run_command("binop xor 2 010203 01 binop_out", 0));
>
> run_command() returns 1 for every failure, including a usage error, so
> these checks cannot tell which validation rejected the input. For
> example, if '*nope' were later accepted as address 0 and the command
> failed for some other reason, the test would still pass. It would be
> worth making this a UTF_CONSOLE test and checking the error message
> (or the usage text) after each command, as other tests in test/cmd do.
> What do you think?
You are right. Updated the binop tests in v2 based on this suggestion.
> Regards,
> Simon
Thanks & Regards,
Maxin
