harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hindess <mark.hind...@googlemail.com>
Subject Re: [classlib][tests] Junit best practice
Date Thu, 26 Oct 2006 18:56:54 GMT

On 26 October 2006 at 16:33, "Alexei Zakharov" <alexei.zakharov@gmail.com> wrote:
> > BTW - the script does run on Windows with ActiveState's Perl runtime.
> > > > Number of Issues by module
> > > >       45 beans
> 
> I've also checked it on WinXP + cygwin + perl v5.8.7. Also works good
> (HARMONY-1976).

Thanks Alexei.  Your patch illustrates an issue that I forgot to
mention...

Another good reason not to automatically fix the "faults" raised by my
script is that they are often indications that there are other similar
but not so easily mechanically-spotted problems with the tests.  For
instance, you might have code like (this is a terrible example, sorry):

  assertEquals(actual, 1);
  assertEquals(actual, expected);

My script will only spot the former because it is an obvious constant,
but a human-being would (hopefully) spot the second when they go to fix
the first one.

Alexei has done a great job of reviewing the tests and fixing the
non-obvious issues.  I especially like the fact that fixing the
exception handling shortens many tests and makes them much more
readable.

Alexei, thanks for taking the time to do this.

Regards,
 Mark.


> 2006/10/26, Nathan Beyer <nbeyer@gmail.com>:
> > BTW - the script does run on Windows with ActiveState's Perl runtime.
> >
> > -Nathatn
> >
> > On 10/25/06, Richard Liang <richard.liangyx@gmail.com> wrote:
> > > Awesome. I just plan to review our junit tests, now it seems your tool
> > > has done what I want ;-)
> > >
> > > I highly recommend we read this article "JUnit best practices"[1] in java
> world.
> > >
> > > [1] http://www.javaworld.com/javaworld/jw-12-2000/jw-1221-junit.html
> > >
> > > On 10/25/06, Mark Hindess <mark.hindess@googlemail.com> wrote:
> > > >
> > > > Earlier in the year we discussed junit best practice.  For example,
> > > > making sure assertEquals calls have the expected and actual arguments
i
> n
> > > > the correct order to avoid getting confusing failure messages.
> > > >
> > > > Robert posted a script a week or so ago, to look for some of junit
> > > > issues but it didn't handle asserts that spanned multiple lines so,
> > > > unfortunately, it was missing the majority of them.  I had a script tha
> t
> > > > I'd thrown together one evening that would handle multi-line asserts bu
> t
> > > > annoyingly (because it read the whole file at once) couldn't report the
> > > > line number of the potential issue as Robert's script did.
> > > >
> > > > Inspired by Robert's post, I looked at my script again.  I've now fixed
> > > > it to report line numbers, added a little bit of documentation and
> > > > attached it to a JIRA:
> > > >
> > > >   https://issues.apache.org/jira/browse/HARMONY-1960
> > > >
> > > > It finds quite a lot of potential problems (I've appended a summary of
> > > > the findings below).  (There will be a few false positives but hopefull
> y
> > > > not too many.)  It would be nice to fix these issues - I fixed several
> > > > hundred while testing the script - but more importantly we should make
> > > > sure we avoid adding any new issues.
> > > >
> > > > Improvements to the script would be most welcome.
> > > >
> > > > Regards,
> > > >  Mark.
> > > >
> > > > Types of issue identified
> > > >
> > > >     4949 should possibly use assertEquals
> > > >      815 actual may be a constant
> > > >      437 consider using separate asserts for each '&&' component
> > > >      330 exception may be left to junit
> > > >      135 actual *may* be a constant
> > > >       48 should be fail (always false)
> > > >       40 should be fail (always true)
> > > >       20 expected is null - should use assertNull
> > > >       12 consider using separate asserts for each '||' component
> > > >        8 expected is false - should use assertFalse
> > > >        7 expected is true - should use assertTrue
> > > >        1 should use assertNotNull
> > > >
> > > >
> > > > Number of Issues by module
> > > >
> > > >     1907 luni
> > > >     1440 swing
> > > >      699 math
> > > >      611 security
> > > >      335 text
> > > >      322 awt
> > > >      222 sound
> > > >      186 nio
> > > >      178 jndi
> > > >      123 archive
> > > >      118 auth
> > > >      117 crypto
> > > >      116 logging
> > > >       91 nio_char
> > > >       87 print
> > > >       74 regex
> > > >       68 concurrent
> > > >       45 beans
> > > >       41 x-net
> > > >       21 sql
> > > >        1 rmi
> 
> 
> -- 
> Alexei Zakharov,
> Intel Enterprise Solutions Software Division
> 





Mime
View raw message