[darcs-users] darcs patch: Add Test.Runner module to run the unit t... (and 5 more)

Reinier Lamers tux_rocker at reinier.de
Wed May 13 20:03:28 UTC 2009


Hi all,

Summary of replies below: thanks for catching the incoherent instances I 
forgot about, I'll fix it. Using a type class for results is a good idea and I 
will try it.

On Wednesday 13 May 2009 19:05:25 Jason Dagit wrote:
> How about we compromise on a partial review now instead of a full review
> later :)

That was one insightful partial review :-)

> Sometimes tests depend on other tests so it may be useful to have a way to
> abort?

Useful, yes, but necessary? It can avoid running tests you know are going to 
fail, but that's not our greatest performance concern now.

> Have you looked at
> the HUnit framework?  You may be reinventing the wheel.

At least there are no test frameworks that allow running tests in parallel, 
which is my I started writing my own.

> > > hunk ./darcs.cabal 642
> > >      RankNTypes,
> > >      GeneralizedNewtypeDeriving,
> > >      MultiParamTypeClasses
> > > +    OverlappingInstances
> > > +    IncoherentInstances
>
> I'm mostly okay with overlapping instances, but incoherent instances
> worries me.  I haven't looked far enough to know why they are there, but
> well, I'm a bit uncomfortable with it :(  (hint: Give me a reason to be
> comfortable.) Would it be possible to use fundeps instead?  Could you point
> me at some reference as that explains your usage is safe?  Also, where do
> you use it?

Oh, great! I enabled them because GHC told me to do it and I forgot to 
investigate it later. AFAIK, the incoherent instances are required because we 
have a rule "instance Testable a => RunnableTest a". If I then define a type A 
and make A a member of both Testable and RunnableTest explicitly, A is a 
member of RunnableTest in two ways. It is a member because of an explicit 
instance declaration, and because it is a Testable, also via the above 
instance declaration for anything Testable.

The solution, as I understand it now, would be to wrap QuickCheck's Testables 
in a special data type (let's call it RunWithQuickCheck):

data RunWithQuickCheck where
    RunWithQuickCheck :: Testable a => a -> RunWithQuickCheck

instance RunnableTest RunWithQuickCheck where ...

This way you don't need the too general rule "instance Testable a => Runnable 
a".

> Perhaps it would be better to use a type class for the result of a
> RunnableTest.

Good point, though atm I like the simplicity of tests that either fail or 
succeed. In later patches, the return type was changed to Maybe String, 
returning an error message upon failure. Making a typeclass that lets you 
query whether it succeeds and what the error message was, is more idiomatic 
imo.

> I haven't yet seen another function in your patch that uses underscores, so
> even this would be preferrable to me, initialRunnerState. This is a minor
> nitpick and I wouldn't want to halt the acceptance of these patches on this
> item.

There is a page on the wiki that tells me to use underscores in local 
functions and camelCase in exported ones.

Or to be precise, http://wiki.darcs.net/index.html/CodingStyle says: "favour 
camelCase for top level functions (instead of the current camel/underscore 
hodgepodge)

    * yes, underscores can be useful in certain situations, don't be 
overzealous
    * In the existing code, camelCase is typically used for names that are 
exported from modules and underscores for names that are internal to a module. 
"

> > > +--   non-standard usage to me, so I don't include it in testrunner.
> > > +instance RunnableTest [String] where
> > > +    run [] = return True
> > > +    run ss = do
> > > +        putStrLn "A test failed! Failure information: "
> > > +        mapM_ putStrLn ss
> > > +        return False
> > >  \end{code}
>
> Hmm...yes I agree with the comment.  This does seem a little odd.  It's as
> if some tests return "Maybe String" where Nothing is success.  What are
> your plans to deal with this in the future?  Refactor the tests that do
> this to be more consistent?

I have no plans in particular.

> > > hunk ./src/Test/Runner.hs 32
> > >
> > >  -- * Classes and types for tests
> > >
> > > +-- | The class of all types that testrunner can treat as a test. The
> >
> > method
> >
> > > +--   'run' should return @Nothing@ if the test succeeds, or @Just s@
> > > if
> >
> > the test
> >
> > > +--   fails, where @s@ is a human-readable description of the failure.
> > >  class RunnableTest a where
> > > hunk ./src/Test/Runner.hs 36
> > > -    run :: a -> IO Bool
> > > +    run :: a -> IO (Maybe String)
>
> This refactor is an example of why I think the result type should be in a
> class :)

Oh, you already saw it. Anyway you are right.

Regards,
Reinier


More information about the darcs-users mailing list