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 ?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
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
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 Software Division -- Tony Wu China Software Development Lab, IBM -- -- Ilya Okomin Intel Enterprise Solutions Software Division
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 ?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
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
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
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 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
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. 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
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 - 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 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
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
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 ?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
[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
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
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
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
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
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
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
