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

[Ann and Jason Edmeades]

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 at gmail.com>wrote:

> On Fri, Dec 14, 2012 at 2:59 AM, Frédéric Delanoy
> <frederic.delanoy at gmail.com> wrote:
> > On Thu, Dec 13, 2012 at 10:59 PM, Ann and Jason Edmeades
> > <jason at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20121214/9b406f78/attachment.html>