Hi Rasmus, On Tue, 17 Mar 2026 at 09:12, Rasmus Villemoes <[email protected]> wrote: > > 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.
That's fine with me, please do. Yes I think I missed v1. Reviewed-by: Simon Glass <[email protected]> Regards, Simon
