[PATCH 1/3] [cmd] Convert the set /a tests to validate better

[Frédéric Delanoy]

On Fri, Dec 14, 2012 at 4:50 PM, Ann and Jason Edmeades
<us at edmeades.me.uk> wrote:
> Hiya,
>
> Thanks for the comments
>
>>I don't really like hardcoding a variable name in a function...
> I don't mind not having the variable hardcoded in the function, but I dont
> like the solution(s), and after all they are only test routines!

Being test routines doesn't mean they don't have to be "clean".
Harcoding variable names is almost alway a bad thing(TM)

>One
> extremely useful benefit of naming the variable and checking the expected
> contents rather than just echoing them, is that you can extract the chunk of
> the test script into a standalone testcase and run/debug that on whatever
> o/s without needing to go through the test infrastructure - whereas getting
> output from the command script and not knowing whether it is right or wrong
> until you go off and compare it to some hard coded list of values is much
> harder to work with.

I don't really understand your point here. You're echoing the variable
name anyway, so if the 'set foo' displays it as well; I don't see the
advantage of your method

> An interim option might be something like 'checkcontents var value var
> value' type routine - would that be better?

Seems convoluted. Why just no go KISS?

>>2. The check for the presence of the first parameter is unneeded since
>>you always call with at least one param (you control all calls)
>
> Its just an example of defensive coding, for which I make no excuse :-)

Well, unneeded here, and to quote you: "after all they are only test
routines" ;-)
Also, the "shift solution" keeps your defensive check at all times
(even if > 3 arguments).

>>3. When only 1 variable is used, no need to add a '1' suffix as in
>>'foo1' or 'WINE_foo1': the '1' doesn't help/add useful information
>
> No, but it doesnt hurt either and gives consistency with the other tests

In other "non-for /a" test, there's never a WINE_foo1 anywhere. Using
"foo", "bar" and "baz" when you have at most 3 arguments seems
standard practice to me.
Using an enumeration suffix for only 1! variable is not reasonable
IMHO (imagine a mathematical "fun(x1)", people might wonder why
there's no x2, x3, ...)

> If the patch doesnt go in as-is, I'll recode the check routine using what I
> describe above and if it does, I'll just patch the routine... If its a
> generic routine as described above it can then be reused elsewhere in the
> tests potentially as well
>
> Thanks for the review!
> Jason
>
You're welcome.

Frédéric

NB: replies on wine-devel should be only the bottom