Re: [PATCH 1/3] [cmd] Convert the set /a tests to validate better
On Thu, Dec 13, 2012 at 10:59 PM, Ann and Jason Edmeades <jason(a)edmeades.me.uk> wrote:
The tests previously set a variable which was not checked until the subsequent test completed (as env vars are expanded as the line is read, meaning a sequence of set /a var=x & echo %var% echos the contents before the set is executed). In addition when multiple variables are involved in the calculation, only the first one was being checked, and this is changed to check all variables involved in the calculation.
+ +REM Check the variables are set to the expected value and clear their contents +:check_vars +if not "%1"=="" ( + if "%1"=="%WINE_var1%" ( + echo WINE_var1 correctly %1 + ) else ( + echo ERROR: WINE_var1 incorrectly %WINE_var1% [%1] + ) +) +if not "%2"=="" ( + if "%2"=="%WINE_var2%" ( + echo WINE_var2 correctly %2 + ) else ( + echo ERROR: WINE_var2 incorrectly %WINE_var2% [%2] + ) +) +if not "%3"=="" ( + if "%3"=="%WINE_var3%" ( + echo WINE_var3 correctly %1 + ) else ( + echo ERROR: WINE_var3 incorrectly %WINE_var3% [%3] + ) +) A couple comments: 1. I don't really like hardcoding a variable name in a function... Why not using something like: set /a var=1 +2 & call :compute var set /a foo=8, bar=foo+1 & call :compute foo bar goto :end :compute set %1 set %1= // to clear the contents if not "%2" == "" ( set %2 set %2= // clearing here as well ) goto :eof :end which prints the variable(s) name and value, without hardcoding? var=3 foo=8 bar=9 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) 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 4. Ideally, for clarity, one would use delayed expansion instead, such as setlocal EnableDelayedExpansion set /a var=1 +2 & echo !var! endlocal but this isn't implemented yet, so adding a preliminary comment like "REM FIXME Simplify tests when EnableDelayedExpansion is supported" would be good IMO 5. You could even simplify the above function (making it more generic, instead of testing for up to 3 arguments) by using SHIFT and an additional label, like :compute :comploop if not "%1" == "" ( set %1 set %1= shift ) else ( goto :eof ) goto :comploop goto :eof Or even better (if NT4 allows): :compute for %%i in (%*) do ( set %%i set %%i= ) goto :eof Frédéric
On Fri, Dec 14, 2012 at 2:59 AM, Frédéric Delanoy <frederic.delanoy(a)gmail.com> wrote:
On Thu, Dec 13, 2012 at 10:59 PM, Ann and Jason Edmeades <jason(a)edmeades.me.uk> wrote:
The tests previously set a variable which was not checked until the subsequent test completed (as env vars are expanded as the line is read, meaning a sequence of set /a var=x & echo %var% echos the contents before the set is executed). In addition when multiple variables are involved in the calculation, only the first one was being checked, and this is changed to check all variables involved in the calculation.
+ +REM Check the variables are set to the expected value and clear their contents +:check_vars +if not "%1"=="" ( + if "%1"=="%WINE_var1%" ( + echo WINE_var1 correctly %1 + ) else ( + echo ERROR: WINE_var1 incorrectly %WINE_var1% [%1] + ) +) +if not "%2"=="" ( + if "%2"=="%WINE_var2%" ( + echo WINE_var2 correctly %2 + ) else ( + echo ERROR: WINE_var2 incorrectly %WINE_var2% [%2] + ) +) +if not "%3"=="" ( + if "%3"=="%WINE_var3%" ( + echo WINE_var3 correctly %1 + ) else ( + echo ERROR: WINE_var3 incorrectly %WINE_var3% [%3] + ) +)
A couple comments:
1. I don't really like hardcoding a variable name in a function... Why not using something like: set /a var=1 +2 & call :compute var set /a foo=8, bar=foo+1 & call :compute foo bar
goto :end :compute
Note "compute" is probably a bad name, "display" might be better to describe the purpose of the routine. Frédéric
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! 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.
An interim option might be something like 'checkcontents var value var value' type routine - would that be better?
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 :-)
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 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 On 14 December 2012 02:01, Frédéric Delanoy <frederic.delanoy(a)gmail.com>wrote:
On Fri, Dec 14, 2012 at 2:59 AM, Frédéric Delanoy <frederic.delanoy(a)gmail.com> wrote:
On Thu, Dec 13, 2012 at 10:59 PM, Ann and Jason Edmeades <jason(a)edmeades.me.uk> wrote:
The tests previously set a variable which was not checked until the subsequent test completed (as env vars are expanded as the line is read, meaning a sequence of set /a var=x & echo %var% echos the contents before the set is executed). In addition when multiple variables are involved in the calculation, only the first one was being checked, and this is changed to check all variables involved in the calculation.
+ +REM Check the variables are set to the expected value and clear their contents +:check_vars +if not "%1"=="" ( + if "%1"=="%WINE_var1%" ( + echo WINE_var1 correctly %1 + ) else ( + echo ERROR: WINE_var1 incorrectly %WINE_var1% [%1] + ) +) +if not "%2"=="" ( + if "%2"=="%WINE_var2%" ( + echo WINE_var2 correctly %2 + ) else ( + echo ERROR: WINE_var2 incorrectly %WINE_var2% [%2] + ) +) +if not "%3"=="" ( + if "%3"=="%WINE_var3%" ( + echo WINE_var3 correctly %1 + ) else ( + echo ERROR: WINE_var3 incorrectly %WINE_var3% [%3] + ) +)
A couple comments:
1. I don't really like hardcoding a variable name in a function... Why not using something like: set /a var=1 +2 & call :compute var set /a foo=8, bar=foo+1 & call :compute foo bar
goto :end :compute
Note "compute" is probably a bad name, "display" might be better to describe the purpose of the routine.
Frédéric
On Fri, Dec 14, 2012 at 4:50 PM, Ann and Jason Edmeades <us(a)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
NB: replies on wine-devel should be only the bottom
Hmm... gmail.... new style compose window.... missed the ...'s - How obvious is that... hopefully sorted? 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
If you have a script which does e.g. set WINE_var1, then it outputs 'WINE_var1=42' or whatever, but is that the right value? With the existing echo behviour its a real pain to know, and relies on you using the make test to run all the suite, then compare the output with some predefined list of results. If you have a script which echo's something like 'wine_var1 correctly 42' or 'ERROR: wine_var1 is 42 and it should be -1' then you immediately see the result. This is especially useful in that you can then extract the portion you need to test into a standalone command file, test on multiple os's, add other tests, then put back into the test suite trivially. Far better than just a straight echo out. I'd also suggest such coding mirrors all the other testsuites, where you check the results you expect then and there, meaning the code and expected values are all side by side.
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?
Because of the above - I very strongly believe the above its far clearer when both writing and debugging - See the cmd copy tests, for example Unfortunately this is the point NT4 kicks in and is a right pain... My ideal solution: :check_vars if "%1"=="" goto :eof for /f "tokens=2 delims==" %%i in ('set %1') do ( if "%%i"=="%2" (echo %1 correctly %2) else echo ERROR: %1 incorrectly %%i [%2] ) set %1= shift shift rem shift goto :check_vars This doesnt work as NT4s for /f is just screwed. The only solution I ended up with is the far from ideal :check_vars if "%1"=="" goto :eof call :executecmd set wine_result=%%%1%% if "%wine_result%"=="%2" ( echo %1 correctly %2 ) else echo ERROR: %1 incorrectly %wine_result% [%2] set %1= shift shift rem shift goto :check_vars :executecmd %* goto :eof Any alternative suggestions? 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.
Ok, I really dont care what they are called - easy enough to fix with a simple search/replace. Any comments on the rest of the patches, especially the code one? Jason
participants (2)
-
Ann and Jason Edmeades -
Frédéric Delanoy