On Tue, Mar 17 2026, Simon Glass <[email protected]> wrote:
> Hi Rasmus,
>
> On Mon, 16 Mar 2026 at 02:17, Rasmus Villemoes <[email protected]> wrote:
>>
>> On Sun, Mar 15 2026, Simon Glass <[email protected]> wrote:
>>
>> > Hi Rasmus,
>> >
>> > On Wed, 11 Mar 2026 at 06:09, Rasmus Villemoes <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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
