You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Mark Hindess <ma...@googlemail.com> on 2006/10/25 14:12:47 UTC
Re: [classlib][tests] Junit best practice
On 25 October 2006 at 7:41, "Geir Magnusson Jr." <ge...@pobox.com> wrote:
>
> Cool - but why not just put into SVN somewhere?
Okay. classlib/trunk/support/tools/bin perhaps?
-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 in
> > 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 that
> > I'd thrown together one evening that would handle multi-line asserts but
> > 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 hopefully
> > 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
> >
> >
> >
> >
>
Re: [classlib][tests] Junit best practice
Posted by Tony Wu <wu...@gmail.com>.
BTW, some of the violation appeared thousands times could be fixed
automatically, do you have any concern?
On 10/26/06, Tony Wu <wu...@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 <wu...@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 <ma...@googlemail.com> wrote:
> > >
> > > On 25 October 2006 at 18:38, "Denis Kishenko" <dk...@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. <ge...@pobox.com>:
> > > > >
> > > > >
> > > > > Mark Hindess wrote:
> > > > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <ge...@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
Re: [classlib][tests] Junit best practice
Posted by Ilya Okomin <il...@gmail.com>.
On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
>
> Hi llya,
> I think you can try to config CheckStyle to exclude files not opened
> in the editor. I believe that will make it faster and avoid oom :)
It's helped, thanks!!
My Eclipse feels much better now;)
Regards,
Ilya.
On 10/27/06, Ilya Okomin <il...@gmail.com> wrote:
> > Mark, Tony thanks for working with this topic.
> > I found it very helpful and necessary for all of us.
> >
> > Tony, I've played with your add-on to CheckStyle plug-in - it works well
> !
> > I'm going to use it in my Eclipse configuration to check future tests.
> >
> > PS One thing in Eclipse is quite annoying. Everything works with small
> > projects but Eclipse fails with OutOfMemory error to me if I switch
> > CheckStyle on in large projects. Did anyone else faced with this error?
> I
> > use Eclipse 3.1.1.
> >
> > Thanks,
> > Ilya.
> >
> > On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
> > >
> > > the configure file of CheckStyle in attachment, you can import it to
> > > your eclipse plugin
> > >
> > > On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
> > > > On 10/26/06, Mark Hindess <ma...@googlemail.com> wrote:
> > > > >
> > > > > On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
> > > > >
> > > > > There's a typo in that last one.
> > > > :p
> > > > > But be careful with these. For
> > > > > example, the second last rule will match things like:
> > > > >
> > > >
> > >
> > modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
> > > > > assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
> > > > Oh, I know why you used [^|&] now.
> > > >
> > > > My problem is that the CheckStyle can not do multi-step check, so I
> > > > have to write rules in one line regexp. For one line regex, there
> are
> > > > many restrictions. It should only be used for assisting manual
> check.
> > > > Your script is better and stricter for auto fixing:)
> > > >
> > > > I tried assertTrue\s*\(.*(?===)&(?!true|false|null).*\)\s*; to match
> a
> > > > string which have == and does not have true, false and null, but it
> > > > did not work:(
> > > > I know you are a regexp guru, do you have some tricks or tips to
> make
> > > > one line regexp match more accurate?
> > > >
> > > > >
> > > > > which is why the regular expressions in my script are a little
> > > stricter
> > > > > (that is not using .* but a character class to avoid catching
> complex
> > > > > cases).
> > > > >
> > > > > > any comments?
> > > > >
> > > > > If you fix any automatically, please check them over manual unless
> you
> > > > > are really, really sure the fixes are not breaking anything.
> > > >
> > > > hmm...I decided to do it manually...
> > > > >
> > > > > Regards,
> > > > > Mark.
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Tony Wu
> > > > China Software Development Lab, IBM
> > > >
> > >
> > >
> > > --
> > > Tony Wu
> > > China Software Development Lab, IBM
> > >
> > >
> > >
> >
> >
> > --
> > --
> > Ilya Okomin
> > Intel Enterprise Solutions Software Division
> >
> >
>
>
> --
> Tony Wu
> China Software Development Lab, IBM
>
--
--
Ilya Okomin
Intel Enterprise Solutions Software Division
Re: [classlib][tests] Junit best practice
Posted by Tony Wu <wu...@gmail.com>.
Hi llya,
I think you can try to config CheckStyle to exclude files not opened
in the editor. I believe that will make it faster and avoid oom :)
On 10/27/06, Ilya Okomin <il...@gmail.com> wrote:
> Mark, Tony thanks for working with this topic.
> I found it very helpful and necessary for all of us.
>
> Tony, I've played with your add-on to CheckStyle plug-in - it works well !
> I'm going to use it in my Eclipse configuration to check future tests.
>
> PS One thing in Eclipse is quite annoying. Everything works with small
> projects but Eclipse fails with OutOfMemory error to me if I switch
> CheckStyle on in large projects. Did anyone else faced with this error? I
> use Eclipse 3.1.1.
>
> Thanks,
> Ilya.
>
> On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
> >
> > the configure file of CheckStyle in attachment, you can import it to
> > your eclipse plugin
> >
> > On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
> > > On 10/26/06, Mark Hindess <ma...@googlemail.com> wrote:
> > > >
> > > > On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
> > > >
> > > > There's a typo in that last one.
> > > :p
> > > > But be careful with these. For
> > > > example, the second last rule will match things like:
> > > >
> > >
> > > modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
> > > > assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
> > > Oh, I know why you used [^|&] now.
> > >
> > > My problem is that the CheckStyle can not do multi-step check, so I
> > > have to write rules in one line regexp. For one line regex, there are
> > > many restrictions. It should only be used for assisting manual check.
> > > Your script is better and stricter for auto fixing:)
> > >
> > > I tried assertTrue\s*\(.*(?===)&(?!true|false|null).*\)\s*; to match a
> > > string which have == and does not have true, false and null, but it
> > > did not work:(
> > > I know you are a regexp guru, do you have some tricks or tips to make
> > > one line regexp match more accurate?
> > >
> > > >
> > > > which is why the regular expressions in my script are a little
> > stricter
> > > > (that is not using .* but a character class to avoid catching complex
> > > > cases).
> > > >
> > > > > any comments?
> > > >
> > > > If you fix any automatically, please check them over manual unless you
> > > > are really, really sure the fixes are not breaking anything.
> > >
> > > hmm...I decided to do it manually...
> > > >
> > > > Regards,
> > > > Mark.
> > > >
> > > >
> > > >
> > >
> > >
> > > --
> > > Tony Wu
> > > China Software Development Lab, IBM
> > >
> >
> >
> > --
> > Tony Wu
> > China Software Development Lab, IBM
> >
> >
> >
>
>
> --
> --
> Ilya Okomin
> Intel Enterprise Solutions Software Division
>
>
--
Tony Wu
China Software Development Lab, IBM
Re: [classlib][tests] Junit best practice
Posted by Tony Wu <wu...@gmail.com>.
Harmony-1997 raised :)
https://issues.apache.org/jira/browse/HARMONY-1997
On 10/27/06, Geir Magnusson Jr. <ge...@pobox.com> wrote:
> can you put this into a JIRA, and maybe some docs, so we can put in SVN
> as a tool for others to use w/ docs on the website?
>
> geir
>
> Tony Wu wrote:
> > the configure file of CheckStyle in attachment, you can import it to
> > your eclipse plugin
> >
> > On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
> >> On 10/26/06, Mark Hindess <ma...@googlemail.com> wrote:
> >> >
> >> > On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
> >> >
> >> > There's a typo in that last one.
> >> :p
> >> > But be careful with these. For
> >> > example, the second last rule will match things like:
> >> >
> >> >
> >> modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
> >>
> >> > assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
> >> Oh, I know why you used [^|&] now.
> >>
> >> My problem is that the CheckStyle can not do multi-step check, so I
> >> have to write rules in one line regexp. For one line regex, there are
> >> many restrictions. It should only be used for assisting manual check.
> >> Your script is better and stricter for auto fixing:)
> >>
> >> I tried assertTrue\s*\(.*(?===)&(?!true|false|null).*\)\s*; to match a
> >> string which have == and does not have true, false and null, but it
> >> did not work:(
> >> I know you are a regexp guru, do you have some tricks or tips to make
> >> one line regexp match more accurate?
> >>
> >> >
> >> > which is why the regular expressions in my script are a little stricter
> >> > (that is not using .* but a character class to avoid catching complex
> >> > cases).
> >> >
> >> > > any comments?
> >> >
> >> > If you fix any automatically, please check them over manual unless you
> >> > are really, really sure the fixes are not breaking anything.
> >>
> >> hmm...I decided to do it manually...
> >> >
> >> > Regards,
> >> > Mark.
> >> >
> >> >
> >> >
> >>
> >>
> >> --
> >> Tony Wu
> >> China Software Development Lab, IBM
> >>
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > <?xml version="1.0" encoding="UTF-8"?>
> > <!--
> > This configuration file was written by the eclipse-cs plugin configuration editor
> > -->
> > <!--
> > Checkstyle-Configuration: test
> > Description:
> >
> > -->
> > <!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.2//EN" "http://www.puppycrawl.com/dtds/configuration_1_2.dtd">
> > <module name="Checker">
> > <property name="severity" value="warning"/>
> > <module name="TreeWalker">
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNull"/>
> > <property name="format" value="assertEquals\s*\((.*,\s*null\s*|\s*null\s*,.*)\);"/>
> > <property name="message" value="should use assertNull"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> > <property name="format" value="assertEquals\s*\((.*,\s*false\s*|\s*false\s*,.*)\);"/>
> > <property name="message" value="should use assertFalse"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> > <property name="format" value="assertEquals\s*\((.*,\s*true\s*|\s*true\s*,.*)\);"/>
> > <property name="message" value="should use assertTrue"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="last argument should not be a constant"/>
> > <property name="format" value="assertEquals\s*\(.*,\s*("[^"]*"|'[^']'|[+-]?\d+[0-9a-fA-FLlPp]*)\s*\);"/>
> > <property name="message" value="last argument should not be a constant"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="always true/false"/>
> > <property name="format" value="assert(False|True)\s*\(\s*(false|true)\s*\)\s*;"/>
> > <property name="message" value="always true/false"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="multiple assertion"/>
> > <property name="format" value="assertTrue\s*\(.*\&\&.*\)\s*;"/>
> > <property name="message" value="multiple assertion"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="multiple assertion"/>
> > <property name="format" value="assertFalse\s*\(.*\|\|.*\)\s*;"/>
> > <property name="message" value="multiple assertion"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNull"/>
> > <property name="format" value="assertTrue\s*\((.*==\s*null\s*|\s*null\s*==.*)\)\s*;"/>
> > <property name="message" value="should use assertNull"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> > <property name="format" value="assertTrue\s*\((.*==\s*true\s*|\s*true\s*==.*)\)\s*;"/>
> > <property name="message" value="should use assertTrue"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> > <property name="format" value="assertTrue\s*\((.*==\s*false\s*|\s*false\s*==.*)\)\s*;"/>
> > <property name="message" value="should use assertFalse"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNotNull"/>
> > <property name="format" value="assertTrue\s*\((.*!=\s*null\s*|\s*null\s*!=.*)\)\s*;"/>
> > <property name="message" value="should use assertNotNull"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> > <property name="format" value="assertTrue\s*\((.*!=\s*true\s*|\s*true\s*!=.*)\)\s*;"/>
> > <property name="message" value="should use assertFalse"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> > <property name="format" value="assertTrue\s*\((.*!=\s*false\s*|\s*false\s*!=.*)\)\s*;"/>
> > <property name="message" value="should use assertTrue"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> > <property name="format" value="assertTrue\s*\(\s*!.*\)\s*;"/>
> > <property name="message" value="should use assertFalse"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> > <property name="format" value="assertTrue\s*\("[^"]*"\s*,\s*!.*\)\s*;"/>
> > <property name="message" value="should use assertFalse"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNotNull"/>
> > <property name="format" value="assertFalse\s*\((.*==\s*null\s*|\s*null\s*==.*)\)\s*;"/>
> > <property name="message" value="should use assertNotNull"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> > <property name="format" value="assertFalse\s*\((.*!=\s*true\s*|\s*true\s*!=.*)\)\s*;"/>
> > <property name="message" value="should use assertFalse"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> > <property name="format" value="assertFalse\s*\((.*!=\s*false\s*|\s*false\s*!=.*)\)\s*;"/>
> > <property name="message" value="should use assertTrue"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNull"/>
> > <property name="format" value="assertFalse\s*\((.*!=\s*null\s*|\s*null\s*!=.*)\)\s*;"/>
> > <property name="message" value="should use assertNull"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> > <property name="format" value="assertFalse\s*\((.*==\s*true\s*|\s*true\s*==.*)\)\s*;"/>
> > <property name="message" value="should use assertFalse"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> > <property name="format" value="assertFalse\s*\((.*==\s*false\s*|\s*false\s*==.*)\)\s*;"/>
> > <property name="message" value="should use assertTrue"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertEquals"/>
> > <property name="format" value="assertTrue\s*\(.*==.*\)\s*;"/>
> > <property name="message" value="should use assertEquals"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > <module name="Regexp">
> > <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertEquals"/>
> > <property name="format" value="assertTrue\s*\(.*\.euqals.*\)\s*;"/>
> > <property name="message" value="should use assertEquals"/>
> > <property name="illegalPattern" value="true"/>
> > <property name="ignoreComments" value="true"/>
> > </module>
> > </module>
> > </module>
>
--
Tony Wu
China Software Development Lab, IBM
Re: [classlib][tests] Junit best practice
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
can you put this into a JIRA, and maybe some docs, so we can put in SVN
as a tool for others to use w/ docs on the website?
geir
Tony Wu wrote:
> the configure file of CheckStyle in attachment, you can import it to
> your eclipse plugin
>
> On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
>> On 10/26/06, Mark Hindess <ma...@googlemail.com> wrote:
>> >
>> > On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
>> >
>> > There's a typo in that last one.
>> :p
>> > But be careful with these. For
>> > example, the second last rule will match things like:
>> >
>> >
>> modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
>>
>> > assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
>> Oh, I know why you used [^|&] now.
>>
>> My problem is that the CheckStyle can not do multi-step check, so I
>> have to write rules in one line regexp. For one line regex, there are
>> many restrictions. It should only be used for assisting manual check.
>> Your script is better and stricter for auto fixing:)
>>
>> I tried assertTrue\s*\(.*(?===)&(?!true|false|null).*\)\s*; to match a
>> string which have == and does not have true, false and null, but it
>> did not work:(
>> I know you are a regexp guru, do you have some tricks or tips to make
>> one line regexp match more accurate?
>>
>> >
>> > which is why the regular expressions in my script are a little stricter
>> > (that is not using .* but a character class to avoid catching complex
>> > cases).
>> >
>> > > any comments?
>> >
>> > If you fix any automatically, please check them over manual unless you
>> > are really, really sure the fixes are not breaking anything.
>>
>> hmm...I decided to do it manually...
>> >
>> > Regards,
>> > Mark.
>> >
>> >
>> >
>>
>>
>> --
>> Tony Wu
>> China Software Development Lab, IBM
>>
>
>
>
> ------------------------------------------------------------------------
>
> <?xml version="1.0" encoding="UTF-8"?>
> <!--
> This configuration file was written by the eclipse-cs plugin configuration editor
> -->
> <!--
> Checkstyle-Configuration: test
> Description:
>
> -->
> <!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.2//EN" "http://www.puppycrawl.com/dtds/configuration_1_2.dtd">
> <module name="Checker">
> <property name="severity" value="warning"/>
> <module name="TreeWalker">
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNull"/>
> <property name="format" value="assertEquals\s*\((.*,\s*null\s*|\s*null\s*,.*)\);"/>
> <property name="message" value="should use assertNull"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> <property name="format" value="assertEquals\s*\((.*,\s*false\s*|\s*false\s*,.*)\);"/>
> <property name="message" value="should use assertFalse"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> <property name="format" value="assertEquals\s*\((.*,\s*true\s*|\s*true\s*,.*)\);"/>
> <property name="message" value="should use assertTrue"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="last argument should not be a constant"/>
> <property name="format" value="assertEquals\s*\(.*,\s*("[^"]*"|'[^']'|[+-]?\d+[0-9a-fA-FLlPp]*)\s*\);"/>
> <property name="message" value="last argument should not be a constant"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="always true/false"/>
> <property name="format" value="assert(False|True)\s*\(\s*(false|true)\s*\)\s*;"/>
> <property name="message" value="always true/false"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="multiple assertion"/>
> <property name="format" value="assertTrue\s*\(.*\&\&.*\)\s*;"/>
> <property name="message" value="multiple assertion"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="multiple assertion"/>
> <property name="format" value="assertFalse\s*\(.*\|\|.*\)\s*;"/>
> <property name="message" value="multiple assertion"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNull"/>
> <property name="format" value="assertTrue\s*\((.*==\s*null\s*|\s*null\s*==.*)\)\s*;"/>
> <property name="message" value="should use assertNull"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> <property name="format" value="assertTrue\s*\((.*==\s*true\s*|\s*true\s*==.*)\)\s*;"/>
> <property name="message" value="should use assertTrue"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> <property name="format" value="assertTrue\s*\((.*==\s*false\s*|\s*false\s*==.*)\)\s*;"/>
> <property name="message" value="should use assertFalse"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNotNull"/>
> <property name="format" value="assertTrue\s*\((.*!=\s*null\s*|\s*null\s*!=.*)\)\s*;"/>
> <property name="message" value="should use assertNotNull"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> <property name="format" value="assertTrue\s*\((.*!=\s*true\s*|\s*true\s*!=.*)\)\s*;"/>
> <property name="message" value="should use assertFalse"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> <property name="format" value="assertTrue\s*\((.*!=\s*false\s*|\s*false\s*!=.*)\)\s*;"/>
> <property name="message" value="should use assertTrue"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> <property name="format" value="assertTrue\s*\(\s*!.*\)\s*;"/>
> <property name="message" value="should use assertFalse"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> <property name="format" value="assertTrue\s*\("[^"]*"\s*,\s*!.*\)\s*;"/>
> <property name="message" value="should use assertFalse"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNotNull"/>
> <property name="format" value="assertFalse\s*\((.*==\s*null\s*|\s*null\s*==.*)\)\s*;"/>
> <property name="message" value="should use assertNotNull"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> <property name="format" value="assertFalse\s*\((.*!=\s*true\s*|\s*true\s*!=.*)\)\s*;"/>
> <property name="message" value="should use assertFalse"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> <property name="format" value="assertFalse\s*\((.*!=\s*false\s*|\s*false\s*!=.*)\)\s*;"/>
> <property name="message" value="should use assertTrue"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertNull"/>
> <property name="format" value="assertFalse\s*\((.*!=\s*null\s*|\s*null\s*!=.*)\)\s*;"/>
> <property name="message" value="should use assertNull"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertFalse"/>
> <property name="format" value="assertFalse\s*\((.*==\s*true\s*|\s*true\s*==.*)\)\s*;"/>
> <property name="message" value="should use assertFalse"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertTrue"/>
> <property name="format" value="assertFalse\s*\((.*==\s*false\s*|\s*false\s*==.*)\)\s*;"/>
> <property name="message" value="should use assertTrue"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertEquals"/>
> <property name="format" value="assertTrue\s*\(.*==.*\)\s*;"/>
> <property name="message" value="should use assertEquals"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> <module name="Regexp">
> <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="should use assertEquals"/>
> <property name="format" value="assertTrue\s*\(.*\.euqals.*\)\s*;"/>
> <property name="message" value="should use assertEquals"/>
> <property name="illegalPattern" value="true"/>
> <property name="ignoreComments" value="true"/>
> </module>
> </module>
> </module>
Re: [classlib][tests] Junit best practice
Posted by Ilya Okomin <il...@gmail.com>.
Mark, Tony thanks for working with this topic.
I found it very helpful and necessary for all of us.
Tony, I've played with your add-on to CheckStyle plug-in - it works well !
I'm going to use it in my Eclipse configuration to check future tests.
PS One thing in Eclipse is quite annoying. Everything works with small
projects but Eclipse fails with OutOfMemory error to me if I switch
CheckStyle on in large projects. Did anyone else faced with this error? I
use Eclipse 3.1.1.
Thanks,
Ilya.
On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
>
> the configure file of CheckStyle in attachment, you can import it to
> your eclipse plugin
>
> On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
> > On 10/26/06, Mark Hindess <ma...@googlemail.com> wrote:
> > >
> > > On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
> > >
> > > There's a typo in that last one.
> > :p
> > > But be careful with these. For
> > > example, the second last rule will match things like:
> > >
> >
> > modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
> > > assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
> > Oh, I know why you used [^|&] now.
> >
> > My problem is that the CheckStyle can not do multi-step check, so I
> > have to write rules in one line regexp. For one line regex, there are
> > many restrictions. It should only be used for assisting manual check.
> > Your script is better and stricter for auto fixing:)
> >
> > I tried assertTrue\s*\(.*(?===)&(?!true|false|null).*\)\s*; to match a
> > string which have == and does not have true, false and null, but it
> > did not work:(
> > I know you are a regexp guru, do you have some tricks or tips to make
> > one line regexp match more accurate?
> >
> > >
> > > which is why the regular expressions in my script are a little
> stricter
> > > (that is not using .* but a character class to avoid catching complex
> > > cases).
> > >
> > > > any comments?
> > >
> > > If you fix any automatically, please check them over manual unless you
> > > are really, really sure the fixes are not breaking anything.
> >
> > hmm...I decided to do it manually...
> > >
> > > Regards,
> > > Mark.
> > >
> > >
> > >
> >
> >
> > --
> > Tony Wu
> > China Software Development Lab, IBM
> >
>
>
> --
> Tony Wu
> China Software Development Lab, IBM
>
>
>
--
--
Ilya Okomin
Intel Enterprise Solutions Software Division
Re: [classlib][tests] Junit best practice
Posted by Tony Wu <wu...@gmail.com>.
the configure file of CheckStyle in attachment, you can import it to
your eclipse plugin
On 10/27/06, Tony Wu <wu...@gmail.com> wrote:
> On 10/26/06, Mark Hindess <ma...@googlemail.com> wrote:
> >
> > On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
> >
> > There's a typo in that last one.
> :p
> > But be careful with these. For
> > example, the second last rule will match things like:
> >
> > modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
> > assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
> Oh, I know why you used [^|&] now.
>
> My problem is that the CheckStyle can not do multi-step check, so I
> have to write rules in one line regexp. For one line regex, there are
> many restrictions. It should only be used for assisting manual check.
> Your script is better and stricter for auto fixing:)
>
> I tried assertTrue\s*\(.*(?===)&(?!true|false|null).*\)\s*; to match a
> string which have == and does not have true, false and null, but it
> did not work:(
> I know you are a regexp guru, do you have some tricks or tips to make
> one line regexp match more accurate?
>
> >
> > which is why the regular expressions in my script are a little stricter
> > (that is not using .* but a character class to avoid catching complex
> > cases).
> >
> > > any comments?
> >
> > If you fix any automatically, please check them over manual unless you
> > are really, really sure the fixes are not breaking anything.
>
> hmm...I decided to do it manually...
> >
> > Regards,
> > Mark.
> >
> >
> >
>
>
> --
> Tony Wu
> China Software Development Lab, IBM
>
--
Tony Wu
China Software Development Lab, IBM
Re: [classlib][tests] Junit best practice
Posted by Tony Wu <wu...@gmail.com>.
On 10/26/06, Mark Hindess <ma...@googlemail.com> wrote:
>
> On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
>
> There's a typo in that last one.
:p
> But be careful with these. For
> example, the second last rule will match things like:
>
> modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
> assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
Oh, I know why you used [^|&] now.
My problem is that the CheckStyle can not do multi-step check, so I
have to write rules in one line regexp. For one line regex, there are
many restrictions. It should only be used for assisting manual check.
Your script is better and stricter for auto fixing:)
I tried assertTrue\s*\(.*(?===)&(?!true|false|null).*\)\s*; to match a
string which have == and does not have true, false and null, but it
did not work:(
I know you are a regexp guru, do you have some tricks or tips to make
one line regexp match more accurate?
>
> which is why the regular expressions in my script are a little stricter
> (that is not using .* but a character class to avoid catching complex
> cases).
>
> > any comments?
>
> If you fix any automatically, please check them over manual unless you
> are really, really sure the fixes are not breaking anything.
hmm...I decided to do it manually...
>
> Regards,
> Mark.
>
>
>
--
Tony Wu
China Software Development Lab, IBM
Re: [classlib][tests] Junit best practice
Posted by Mark Hindess <ma...@googlemail.com>.
On 26 October 2006 at 19:16, "Tony Wu" <wu...@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*;
There's a typo in that last one. But be careful with these. For
example, the second last rule will match things like:
modules/luni/src/test/java/tests/api/java/io/BufferedInputStreamTest.java:
assertTrue("Wrong bytes", in.read() == 6 && in.read() == 7);
which is why the regular expressions in my script are a little stricter
(that is not using .* but a character class to avoid catching complex
cases).
> any comments?
If you fix any automatically, please check them over manual unless you
are really, really sure the fixes are not breaking anything.
Regards,
Mark.
Re: [classlib][tests] Junit best practice
Posted by Tony Wu <wu...@gmail.com>.
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 <wu...@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 <ma...@googlemail.com> wrote:
> >
> > On 25 October 2006 at 18:38, "Denis Kishenko" <dk...@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. <ge...@pobox.com>:
> > > >
> > > >
> > > > Mark Hindess wrote:
> > > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <ge...@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
Re: [classlib][tests] Junit best practice
Posted by Tony Wu <wu...@gmail.com>.
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 <ma...@googlemail.com> wrote:
>
> On 25 October 2006 at 18:38, "Denis Kishenko" <dk...@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. <ge...@pobox.com>:
> > >
> > >
> > > Mark Hindess wrote:
> > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <ge...@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
Re: [classlib][tests] Junit best practice
Posted by Mark Hindess <ma...@googlemail.com>.
On 25 October 2006 at 18:38, "Denis Kishenko" <dk...@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. <ge...@pobox.com>:
> >
> >
> > Mark Hindess wrote:
> > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <ge...@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
>
Re: [classlib][tests] Junit best practice
Posted by Denis Kishenko <dk...@gmail.com>.
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 =)
2006/10/25, Geir Magnusson Jr. <ge...@pobox.com>:
>
>
> Mark Hindess wrote:
> > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <ge...@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 in
> >>> 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 that
> >>> I'd thrown together one evening that would handle multi-line asserts but
> >>> 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 hopefully
> >>> 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
Re: [classlib][tests] Junit best practice
Posted by "Geir Magnusson Jr." <ge...@pobox.com>.
Mark Hindess wrote:
> On 25 October 2006 at 7:41, "Geir Magnusson Jr." <ge...@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 in
>>> 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 that
>>> I'd thrown together one evening that would handle multi-line asserts but
>>> 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 hopefully
>>> 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
>>>
>>>
>>>
>>>
>
>
>