Re: [PATCH 2/3] test: add tests for left-bracket alias for 'test' command

[Rasmus Villemoes <ravi@prevas.dk>]

On Tue, Mar 17 2026, Simon Glass <sjg@chromium.org> wrote:

> Hi Rasmus,
>
> On Mon, 16 Mar 2026 at 02:17, Rasmus Villemoes <ravi@prevas.dk> wrote:
>>
>> On Sun, Mar 15 2026, Simon Glass <sjg@chromium.org> wrote:
>>
>> > Hi Rasmus,
>> >
>> > On Wed, 11 Mar 2026 at 06:09, Rasmus Villemoes <ravi@prevas.dk> wrote:
>> >>
>> >> Duplicate a few of the existing test cases, using the [ spelling, and
>> >> also ensure that the presence of a matching ] as a separate and last
>> >> argument is enforced.
>> >>
>> >> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>> >> ---
>> >>  test/hush/if.c | 31 +++++++++++++++++++++++++++++++
>> >>  1 file changed, 31 insertions(+)
>> >>
>> >> diff --git a/test/hush/if.c b/test/hush/if.c
>> >> index ea615b246a9..148e9a53e90 100644
>> >> --- a/test/hush/if.c
>> >> +++ b/test/hush/if.c
>> >> @@ -315,3 +315,34 @@ static int hush_test_if_z_operator(struct unit_test_state *uts)
>> >>         return 0;
>> >>  }
>> >>  HUSH_TEST(hush_test_if_z_operator, 0);
>> >> +
>> >> +static int hush_test_lbracket_alias(struct unit_test_state *uts)
>> >> +{
>> >> +       char if_formatted[128];
>> >> +       const char *missing_rbracket_error = "[: missing terminating ]";
>> >> +
>> >> +       sprintf(if_formatted, if_format, "[ aaa = aaa ]");
>> >> +       ut_assertok(run_command(if_formatted, 0));
>> >
>> > How about using run_commandf() so you can do this in one line? Looks
>> > good apart from that.
>> >
>>
>> I did cringe a little when I saw that repeated sprintf()/run_command()
>> pattern all over that file, but I preferred to stay consistent with the
>> existing style.
>>
>> If anything, one should do a whole-sale conversion of that file, but
>> then I'd not use run_commandf() directly, as one would still pass in
>> that if_format argument every time, so rather create a local macro that
>> wraps run_commandf() and provides that if_format as a literal string,
>> which would also enable format checking.
>
> Well you could have a helper function which takes the string argument
> and does the assert. The duplication makes it a bit harder to
> maintain.
>
> sprintf(if_formatted, if_format, "test aaa != aaa");
> ut_asserteq(1, run_command(if_formatted, 0));
>
> could be something like:
>
> ut_asserteq(1, check_operation("test aaa != aaa"));

Yes, I'd definitely make sure that each test would only occupy one
source line instead of two. What I meant was that I didn't want to
convert

    sprintf(if_formatted, if_format, "test aaa = aaa");
    ut_assertok(run_command(if_formatted, 0));

mechanically into

   ut_assertok(run_commandf(if_format, "test aaa = aaa", 0));

and having to repeat that if_format thing on each and every line.

Also, I think I would prefer to change the assertok()s into asserteq(0,
... ) as I think the mix of assertok() and asserteq(1, ...) is not very
readable.

> If you had the energy to adjust the rest of the file, that seems good too.

I will fix that in a followup if the current patches get merged (they're
at v2 by the way), I do not have the energy to start over and doing the
mechanical changes first and then redo my patches on top of that, so
please don't request that I do that.

Rasmus