Re: [classlib][tests] Junit best practice
On 11/2/06, Tony Wu <[EMAIL PROTECTED]> wrote:
hey guys,
I found there're same problem for assertSame like assertEquals.
should use assertNull, assertSame\s*\((.*,\s*null\s*|\s*null\s*,.*)\)\s*;
should use assertFalse, assertSame\s*\((.*,\s*false\s*|\s*false\s*,.*)\)\s*;
should use assertTrue, assertSame\s*\((.*,\s*true\s*|\s*true\s*,.*)\)\s*;
last argument should not be a constant,
assertSame\s*\(.*,\s*("[^"]*"|'[^']'|[+-]?\d+[0-9a-fA-FLlPp]*|[A-Z_]*)\s*\)\s*;
I'll update my checklist if no one object.
Please go for it ;-)
On 10/25/06, Mark Hindess <[EMAIL PROTECTED]> 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
>
>
>
>
--
Tony Wu
China Software Development Lab, IBM
--
Richard Liang
China Development Lab, IBM
Re: [classlib][tests] Junit best practice
hey guys,
I found there're same problem for assertSame like assertEquals.
should use assertNull, assertSame\s*\((.*,\s*null\s*|\s*null\s*,.*)\)\s*;
should use assertFalse, assertSame\s*\((.*,\s*false\s*|\s*false\s*,.*)\)\s*;
should use assertTrue, assertSame\s*\((.*,\s*true\s*|\s*true\s*,.*)\)\s*;
last argument should not be a constant,
assertSame\s*\(.*,\s*("[^"]*"|'[^']'|[+-]?\d+[0-9a-fA-FLlPp]*|[A-Z_]*)\s*\)\s*;
I'll update my checklist if no one object.
On 10/25/06, Mark Hindess <[EMAIL PROTECTED]> 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
--
Tony Wu
China Software Development Lab, IBM
Re: [classlib][tests] Junit best practice
Harmony-1997 raised :)
https://issues.apache.org/jira/browse/HARMONY-1997
On 10/27/06, Geir Magnusson Jr. <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> wrote:
>> On 10/26/06, Mark Hindess <[EMAIL PROTECTED]> wrote:
>> >
>> > On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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
>>
>
>
>
>
>
>
>
>
> http://www.puppycrawl.com/dtds/configuration_1_2.dtd";>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
Re: [classlib][tests] Junit best practice
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 <[EMAIL PROTECTED]> wrote:
On 10/26/06, Mark Hindess <[EMAIL PROTECTED]> wrote:
>
> On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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
http://www.puppycrawl.com/dtds/configuration_1_2.dtd";>
Re: [classlib][tests] Junit best practice
On 10/27/06, Tony Wu <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> wrote:
> >
> > the configure file of CheckStyle in attachment, you can import it to
> > your eclipse plugin
> >
> > On 10/27/06, Tony Wu <[EMAIL PROTECTED]> wrote:
> > > On 10/26/06, Mark Hindess <[EMAIL PROTECTED]> wrote:
> > > >
> > > > On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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 Softwa
Re: [classlib][tests] Junit best practice
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> wrote:
>
> the configure file of CheckStyle in attachment, you can import it to
> your eclipse plugin
>
> On 10/27/06, Tony Wu <[EMAIL PROTECTED]> wrote:
> > On 10/26/06, Mark Hindess <[EMAIL PROTECTED]> wrote:
> > >
> > > On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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
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 <[EMAIL PROTECTED]> wrote:
the configure file of CheckStyle in attachment, you can import it to
your eclipse plugin
On 10/27/06, Tony Wu <[EMAIL PROTECTED]> wrote:
> On 10/26/06, Mark Hindess <[EMAIL PROTECTED]> wrote:
> >
> > On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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
the configure file of CheckStyle in attachment, you can import it to
your eclipse plugin
On 10/27/06, Tony Wu <[EMAIL PROTECTED]> wrote:
On 10/26/06, Mark Hindess <[EMAIL PROTECTED]> wrote:
>
> On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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
http://www.puppycrawl.com/dtds/configuration_1_2.dtd";>
Re: [classlib][tests] Junit best practice
On 10/26/06, Mark Hindess <[EMAIL PROTECTED]> wrote:
On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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
On 26 October 2006 at 16:33, "Alexei Zakharov" <[EMAIL PROTECTED]> wrote: > > BTW - the script does run on Windows with ActiveState's Perl runtime. > > > > Number of Issues by module > > > > 45 beans > > I've also checked it on WinXP + cygwin + perl v5.8.7. Also works good > (HARMONY-1976). Thanks Alexei. Your patch illustrates an issue that I forgot to mention... Another good reason not to automatically fix the "faults" raised by my script is that they are often indications that there are other similar but not so easily mechanically-spotted problems with the tests. For instance, you might have code like (this is a terrible example, sorry): assertEquals(actual, 1); assertEquals(actual, expected); My script will only spot the former because it is an obvious constant, but a human-being would (hopefully) spot the second when they go to fix the first one. Alexei has done a great job of reviewing the tests and fixing the non-obvious issues. I especially like the fact that fixing the exception handling shortens many tests and makes them much more readable. Alexei, thanks for taking the time to do this. Regards, Mark. > 2006/10/26, Nathan Beyer <[EMAIL PROTECTED]>: > > BTW - the script does run on Windows with ActiveState's Perl runtime. > > > > -Nathatn > > > > On 10/25/06, Richard Liang <[EMAIL PROTECTED]> wrote: > > > Awesome. I just plan to review our junit tests, now it seems your tool > > > has done what I want ;-) > > > > > > I highly recommend we read this article "JUnit best practices"[1] in java > world. > > > > > > [1] http://www.javaworld.com/javaworld/jw-12-2000/jw-1221-junit.html > > > > > > On 10/25/06, Mark Hindess <[EMAIL PROTECTED]> wrote: > > > > > > > > Earlier in the year we discussed junit best practice. For example, > > > > making sure assertEquals calls have the expected and actual arguments i > n > > > > the correct order to avoid getting confusing failure messages. > > > > > > > > Robert posted a script a week or so ago, to look for some of junit > > > > issues but it didn't handle asserts that spanned multiple lines so, > > > > unfortunately, it was missing the majority of them. I had a script tha > t > > > > I'd thrown together one evening that would handle multi-line asserts bu > t > > > > annoyingly (because it read the whole file at once) couldn't report the > > > > line number of the potential issue as Robert's script did. > > > > > > > > Inspired by Robert's post, I looked at my script again. I've now fixed > > > > it to report line numbers, added a little bit of documentation and > > > > attached it to a JIRA: > > > > > > > > https://issues.apache.org/jira/browse/HARMONY-1960 > > > > > > > > It finds quite a lot of potential problems (I've appended a summary of > > > > the findings below). (There will be a few false positives but hopefull > y > > > > not too many.) It would be nice to fix these issues - I fixed several > > > > hundred while testing the script - but more importantly we should make > > > > sure we avoid adding any new issues. > > > > > > > > Improvements to the script would be most welcome. > > > > > > > > Regards, > > > > Mark. > > > > > > > > Types of issue identified > > > > > > > > 4949 should possibly use assertEquals > > > > 815 actual may be a constant > > > > 437 consider using separate asserts for each '&&' component > > > > 330 exception may be left to junit > > > > 135 actual *may* be a constant > > > > 48 should be fail (always false) > > > > 40 should be fail (always true) > > > > 20 expected is null - should use assertNull > > > > 12 consider using separate asserts for each '||' component > > > >8 expected is false - should use assertFalse > > > >7 expected is true - should use assertTrue > > > >1 should use assertNotNull > > > > > > > > > > > > Number of Issues by module > > > > > > > > 1907 luni > > > > 1440 swing > > > > 699 math > > > > 611 security > > > > 335 text > > > > 322 awt > > > > 222 sound > > > > 186 nio > > > > 178 jndi > > > > 123 archive > > > > 118 auth > > > > 117 crypto > > > > 116 logging > > > > 91 nio_char > > > > 87 print > > > > 74 regex > > > > 68 concurrent > > > > 45 beans > > > > 41 x-net > > > > 21 sql > > > >1 rmi > > > -- > Alexei Zakharov, > Intel Enterprise Solutions Software Division >
Re: [classlib][tests] Junit best practice
BTW - the script does run on Windows with ActiveState's Perl runtime. > > Number of Issues by module > > 45 beans I've also checked it on WinXP + cygwin + perl v5.8.7. Also works good (HARMONY-1976). Thanks, 2006/10/26, Nathan Beyer <[EMAIL PROTECTED]>: BTW - the script does run on Windows with ActiveState's Perl runtime. -Nathatn On 10/25/06, Richard Liang <[EMAIL PROTECTED]> wrote: > Awesome. I just plan to review our junit tests, now it seems your tool > has done what I want ;-) > > I highly recommend we read this article "JUnit best practices"[1] in javaworld. > > [1] http://www.javaworld.com/javaworld/jw-12-2000/jw-1221-junit.html > > On 10/25/06, Mark Hindess <[EMAIL PROTECTED]> 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 -- Alexei Zakharov, Intel Enterprise Solutions Software Division
Re: [classlib][tests] Junit best practice
On 26 October 2006 at 19:16, "Tony Wu" <[EMAIL PROTECTED]> 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
BTW, some of the violation appeared thousands times could be fixed
automatically, do you have any concern?
On 10/26/06, Tony Wu <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> wrote:
> >
> > On 25 October 2006 at 18:38, "Denis Kishenko" <[EMAIL PROTECTED]> 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. <[EMAIL PROTECTED]>:
> > > >
> > > >
> > > > Mark Hindess wrote:
> > > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <[EMAIL PROTECTED]>
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.
> > > > >>>
> >
Re: [classlib][tests] Junit best practice
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> wrote:
>
> On 25 October 2006 at 18:38, "Denis Kishenko" <[EMAIL PROTECTED]> 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. <[EMAIL PROTECTED]>:
> > >
> > >
> > > Mark Hindess wrote:
> > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <[EMAIL PROTECTED]>
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 quit
Re: [classlib][tests] Junit best practice
Great, Mark. Your script is indeed so powerful! On 10/25/06, Mark Hindess <[EMAIL PROTECTED]> 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 -- Robert Hu China Software Development Lab, IBM
Re: [classlib][tests] Junit best practice
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 <[EMAIL PROTECTED]> wrote: On 25 October 2006 at 18:38, "Denis Kishenko" <[EMAIL PROTECTED]> 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. <[EMAIL PROTECTED]>: > > > > > > Mark Hindess wrote: > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <[EMAIL PROTECTED]> 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
BTW - the script does run on Windows with ActiveState's Perl runtime. -Nathatn On 10/25/06, Richard Liang <[EMAIL PROTECTED]> wrote: Awesome. I just plan to review our junit tests, now it seems your tool has done what I want ;-) I highly recommend we read this article "JUnit best practices"[1] in javaworld. [1] http://www.javaworld.com/javaworld/jw-12-2000/jw-1221-junit.html On 10/25/06, Mark Hindess <[EMAIL PROTECTED]> 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 > > > > -- Richard Liang China Development Lab, IBM
Re: [classlib][tests] Junit best practice
Awesome. I just plan to review our junit tests, now it seems your tool has done what I want ;-) I highly recommend we read this article "JUnit best practices"[1] in javaworld. [1] http://www.javaworld.com/javaworld/jw-12-2000/jw-1221-junit.html On 10/25/06, Mark Hindess <[EMAIL PROTECTED]> 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 -- Richard Liang China Development Lab, IBM
Re: [classlib][tests] Junit best practice
On 25 October 2006 at 18:38, "Denis Kishenko" <[EMAIL PROTECTED]> 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. <[EMAIL PROTECTED]>: > > > > > > Mark Hindess wrote: > > > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <[EMAIL PROTECTED]> > > > 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
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. <[EMAIL PROTECTED]>: Mark Hindess wrote: > On 25 October 2006 at 7:41, "Geir Magnusson Jr." <[EMAIL PROTECTED]> 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
Mark Hindess wrote: On 25 October 2006 at 7:41, "Geir Magnusson Jr." <[EMAIL PROTECTED]> 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
Re: [classlib][tests] Junit best practice
On 25 October 2006 at 7:41, "Geir Magnusson Jr." <[EMAIL PROTECTED]> 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
Cool - but why not just put into SVN somewhere? 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
[classlib][tests] Junit best practice
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
