[darcs-devel] [patch761] introduce darcs test, doing the same as ... (and 5 more)

[Michael Hendricks]

Michael Hendricks <michael at ndrix.org> added the comment:

Hi Guillaume.  Thanks for patches.  Since I said I thought this was a good idea, it 
seems only fair that I do some code review too :)

> hunk ./src/Darcs/Commands/Test.hs
> +test = DarcsCommand  {commandProgramName = "darcs",
> +                      commandName = "test",
> +                      commandHelp = testHelp,
> +                      commandDescription = testDescription,
> +                      commandExtraArgs = 0,
> +                      commandExtraArgHelp = [],
> +                      commandCommand = testCmd,
> +                      commandPrereq = amInHashedRepository,
> +                      commandGetArgPossibilities = return [],
> +                      commandArgdefaults = nodefaults,
> +                      commandAdvancedOptions = [],
> +                      commandBasicOptions = [ leaveTestDir,
> +                                              workingRepoDir
> +                                             ]}

A good opportunity to rearrange following the style guide.

> hunk ./tests/oldfashioned_refusal.sh 56
> -not darcs trackdown

Should this add a line "not darcs test"?

> hunk ./tests/trackdown-bisect.sh 35
> -# You can remove --bisect for compare with linear trackdown
> -trackdown_args='--bisect' $
> +# You can replace --bisect by --trackdown for compare with linear trackdown
> +test_args='--bisect' $

A good chance to remove the trailing whitespace on these lines.

> hunk ./tests/trackdown-bisect.sh 131
> -trackdown_args='--bisect' $
> +test_args='--bisect' $

Trailing whitespace.

> hunk ./src/Darcs/Arguments.hs
> +__trackdown, __bisect :: DarcsAtomicOption
> +__trackdown = DarcsNoArgOption [] ["trackdown"] TrackDown
> +                "locate the most recent version lacking an error"
> +__bisect = DarcsNoArgOption [] ["bisect"] Bisect
> +             "binary instead of linear search"

I'd really like to see --trackdown renamed to --linear and a new option --once (for 
the default behavior).  Having --once lets one override on the command line "test -
-bisect" in a defaults file.  This naming convention makes it clearer, to my eyes, 
that one is specifying a search strategy.  --strategy={once,linear,bisect} would be 
even better, but maybe that's too different from the rest of darcs options.

I've been working on a trackdown strategy that starts at head, backing off 
exponentially until the test passes, then bisecting the skipped over patches.  
While working on that patch, the absence of `trackdown --linear` stood out as a UI 
flaw.

> hunk ./src/Darcs/Commands/Test.hs 97
> - "If a regression test is defined (see `darcs setpref') it will be run.\n"
> + unlines
> + [ "Run test on the current recorded state of the repository.  Given no"

Great to see unlines here.

> +  ,"is an initialization command with is run only once, and the second is"

s/with is/which is/

> +  if once
> +   then withRepository (Test:opts) $ RepoJob $ oneTest opts
> +   else withRecorded repository (withTempDir "trackingdown") $ \_ -> do

I'd personally like to see `darcs test --once` perform its test in a clean 
checkout, like --linear and --bisect do.  Although that's a departure from `darcs 
check` behavior, so probably out of line for a code review anyway.

> +      currProg = (1, 1 + round ((logBase 2 $ fromIntegral $ lengthRL ps) :: 
Double)) :: (Int,Int)

More precisely, ":: BisectState"

> +# ensure no warning message is displayed when not giving an initialization
> +# command to darcs test without --trackdown nor --bisect flags
> +darcs test --trackdown true true | grep -v WARNING
> +
> +# ensure no warning message is displayed when not giving an initialization
> +# command to darcs test without --trackdown nor --bisect flags
> +darcs test --bisect true | grep -v WARNING

It took me quite a while to understand the comments because of all the negatives.  
Is the following clearer to anyone else?

  ensure no warning message is displayed when giving an initialization
  command to darcs test with --trackdown or --bisect flags

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch761>
__________________________________