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