On Fri, Dec 14, 2012 at 4:50 PM, Ann and Jason Edmeades us@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?
- 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).
- 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