[darcs-users] [patch371] Implement a test-framework-based shell h... (and 3 more)

[Simon Michael]

On 8/29/10 6:55 AM, Petr Ročkai wrote:
> Latest iteration of the patches. Shellish is on Hackage now, so nothing
> substantial should stand in the way of this now.

Cool. Providing what review I can for now, covering the first two patches:

> [Implement a test-framework-based shell harness.
> Petr Rockai <me at mornfall.net>**20100827204448
>  Ignore-this: 819e700538a38bf35e04eb7656b3c151
>
>  This merges the two test drivers we previously had, "unit" and
>  Distribution.ShellHarness (used directly by Setup), into a single "darcs-test"

+1

> hunk ./Distribution/ShellHarness.hs 64
>                                   Just d  -> return $ takeDirectory d
>                      Just x -> return x
>      let myenv = [("HOME",cwd)
> +                ,("TESTDATA",cwd </> "data")
> +                ,("TESTBIN",cwd </> "bin")

I guess DARCSSOURCE/tests/{data,bin} directories will be created.. hopefully it doesn't require you to be in a 
particular directory to run tests.

> hunk ./darcs.cabal 604
>                       filepath     == 1.1.*,
>                       QuickCheck   >= 2.1.0.0,
>                       HUnit        >= 1.0,
> +                     cmdlib       >= 0.2.1 && < 0.3,

-1. I know you have your reasons and this is just the test runner, not darcs itself, but I think cmdargs will succeed 
and it would be better to use/improve that.

> +                     shellish     == 0.1.*,

The hackage page says "safety is sacrificed". Anything to be concerned about ? Will darcs tests built on this be 
reliable and deterministic ?

>                       test-framework             >= 0.2.2,
>                       test-framework-hunit       >= 0.2.2,
>                       test-framework-quickcheck2 >= 0.2.2

+1. However, new packages (the 5 above) bring compatibility headaches. It looks like all these "should" work on windows, 
but until tested we should probably assume extra installation hassles for darcs developers on that platform. Likewise 
for folks wanting to build tests with GHC 6.10.

> +data ShellTest = ShellTest { format :: Format
> +                           , file :: FilePath
> +                           , darcspath :: FilePath }

Same name as the generic ShellTest in shelltestrunner, oh well. Following this is a simple darcs-specific test runner, 
with slight shelltestrunner overlap but nothing to worry about codewise. I think it is worth reviewing and comparing the 
cost/benefit of current darcs shell tests and shelltestrunner-style tests, perhaps there's some extra value to be had. 
But I have not done that.

Finally, there's a bunch of adjustments to tests to use local directories. Looks good.

Summary: a good, high-value change, with concerns about the use of cmdlib and about unclear platform/compiler 
compatibility impact.