harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tony Wu" <wuyue...@gmail.com>
Subject Re: [classlib][tests] Junit best practice
Date Thu, 26 Oct 2006 11:20:41 GMT
BTW, some of the violation appeared thousands times could be fixed
automatically, do you have any concern?

On 10/26/06, Tony Wu <wuyuehao@gmail.com> wrote:
> I have scratched out the stand alone rules,
>
> should use assertNull, assertEquals\s*\((.*,\s*null\s*|\s*null\s*,.*)\)\s*;
> should use assertFalse, assertEquals\s*\((.*,\s*false\s*|\s*false\s*,.*)\)\s*;
> should use assertTrue, assertEquals\s*\((.*,\s*true\s*|\s*true\s*,.*)\)\s*;
> last argument should not be a constant,
> assertEquals\s*\(.*,\s*("[^"]*"|'[^']'|[+-]?\d+[0-9a-fA-FLlPp]*|[A-Z_]*)\s*\)\s*;
> always true/false, assert(False|True)\s*\(\s*(false|true)\s*\)\s*;
> multiple assertion, assertTrue\s*\(.*\&\&.*\)\s*;
> multiple assertion, assertFalse\s*\(.*\|\|.*\)\s*;
> should use assertNull, assertTrue\s*\((.*==\s*null\s*|\s*null\s*==.*)\)\s*;
> should use assertTrue, assertTrue\s*\((.*==\s*true\s*|\s*true\s*==.*)\)\s*;
> should use assertFalse, assertTrue\s*\((.*==\s*false\s*|\s*false\s*==.*)\)\s*;
> should use assertNotNull, assertTrue\s*\((.*!=\s*null\s*|\s*null\s*!=.*)\)\s*;
> should use assertFalse, assertTrue\s*\((.*!=\s*true\s*|\s*true\s*!=.*)\)\s*;
> should use assertTrue, assertTrue\s*\((.*!=\s*false\s*|\s*false\s*!=.*)\)\s*;
> should use assertFalse, assertTrue\s*\(\s*!.*\)\s*;
> should use assertFalse, assertTrue\s*\("[^"]*"\s*,\s*!.*\)\s*;
> should use assertNotNull, assertFalse\s*\((.*==\s*null\s*|\s*null\s*==.*)\)\s*;
> should use assertFalse, assertFalse\s*\((.*!=\s*true\s*|\s*true\s*!=.*)\)\s*;
> should use assertTrue, assertFalse\s*\((.*!=\s*false\s*|\s*false\s*!=.*)\)\s*;
> should use assertNull, assertFalse\s*\((.*!=\s*null\s*|\s*null\s*!=.*)\)\s*;
> should use assertFalse, assertFalse\s*\((.*==\s*true\s*|\s*true\s*==.*)\)\s*;
> should use assertTrue, assertFalse\s*\((.*==\s*false\s*|\s*false\s*==.*)\)\s*;
> should use assertEquals, assertTrue\s*\(.*==.*\)\s*;
> should use assertEquals, assertTrue\s*\(.*\.euqals.*\)\s*;
>
> any comments?
>
> I'll add to my CheckStyle and dispalyed them as warning.
>
> On 10/26/06, Tony Wu <wuyuehao@gmail.com> wrote:
> > Instead of fixing them by script, for those who use eclipse, I suggest
> > to use CheckStyle plugin and set up rules according Mark's perl
> > script. It will highlight the code which breaks the rules with a
> > specified comment. It is easy for manual fixing I think.
> >
> > On 10/25/06, Mark Hindess <mark.hindess@googlemail.com> wrote:
> > >
> > > On 25 October 2006 at 18:38, "Denis Kishenko" <dkishenko@gmail.com> wrote:
> > > >
> > > > Mark, I have just tried your tool. It's really helpful, thanks a lot!
> > > >
> > > > It's so pitty that script doesn't fix issues by itself =)
> > >
> > > It could (and I have been known to use scripts to fix things) but as
> > > Nathan recently pointed out.  It's not always a good idea.
> > >
> > > Specifically, my tool is not perfect at identifying the different types
> > > of assertEquals calls (e.g. the variants for double where the last
> > > argument might look like a constant but isn't the expected value but a
> > > delta).  It does a reasonable job but without implementing a full parser
> > > it's never going to be 100% reliable.
> > >
> > > I did use a script - a one-off on the command line - to fix a significant
> > > number of assertEquals calls to use assertTrue/assertFalse/assertNull a
> > > week or so ago, but I did also scan the diff to see if it looked sane.
> > > Scanning the diff was almost as tedious as fixing them manually. ;-(
> > >
> > > Regards,
> > >  Mark.
> > >
> > > > 2006/10/25, Geir Magnusson Jr. <geir@pobox.com>:
> > > > >
> > > > >
> > > > > Mark Hindess wrote:
> > > > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <geir@pobox.com>
wrote:
> > > > > >> Cool - but why not just put into SVN somewhere?
> > > > > >
> > > > > > Okay.  classlib/trunk/support/tools/bin perhaps?
> > > > >
> > > > > Sure.  Whatever you feel is best. I have no strong opinion.  We do
have
> > > > > junit tests in DRLVM too, but we can "reach over" and use from there
for
> > > > > now.
> > > > >
> > > > > geir
> > > > >
> > > > > >
> > > > > > -Mark.
> > > > > >
> > > > > >> either in enhanced/tools or classlib/trunk somewhere where
it can be
> > > > > >> invoked as an option by people from ant (so that we can
wire it into the
> > > > > >> CI system...)
> > > > > >>
> > > > > >> geir
> > > > > >>
> > > > > >>
> > > > > >> Mark Hindess 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
> > > > > >>>
> > > > > >>>
> > > > > >>>
> > > > > >>>
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Denis M. Kishenko
> > > > Intel Middleware Products Division
> > > >
> > >
> > >
> > >
> >
> >
> > --
> > Tony Wu
> > China Software Development Lab, IBM
> >
>
>
> --
> Tony Wu
> China Software Development Lab, IBM
>


-- 
Tony Wu
China Software Development Lab, IBM

Mime
View raw message