[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