RFR 7902161 : Discover simple getters, setters, throwers and delegators.

[Alexandre (Shura) Iline]

Thank you very much for the discovered inconsistencies and problems. With the exception of one, all fixed. 

> On May 9, 2018, at 5:24 PM, Alexey Fedorchenko <alexey.fedorchenko at oracle.com> wrote:
> 
> Thanks for the update!
> The tests’ approach (to verify the test class itself) looks nice to me.
> 
> A few things:
> 
> - please, add the author to the readme file.

Done

> 
> - usage string has two names for the plugin’s jar (readme and Scanner.java): 
>        "java -classpath jcov.jar:SimpleMethods.jar --usage\n" +
>        "\n" +
>        "java -classpath jcov.jar:simple_methods_anc.jar “ 

Fixed.

> 	(btw do we need 2 lines here? The first command will not succeed.)

Not sure what you mean. Does passing —usage fail?

> 
> - include/exclude options (long and short names) differs in readme and in command line. 

Fixed.

> 
> - if no filters are specified the output differs from the command line options:
> One or more of --simple getter,--simple setter,--simple delegator,--simple thrower,--empty methods options must be specified 

Fixed.

> 
> - unused methods in Utils.java (just to pay you attention,may be it is static methods for future usage) 
> and lastDot variable in the visitClass method (Scanner.java) 

Leftover from an old algorithm. Removed.
 
> 
> - testng.jar property is not enough to run tests, users will need to add jcommander to the classpath (“test” target is not executable).

Fixed.

> 
> - command without uri schema will give NullPointerException 
> (java -jar jcov.jar:SimpleMethods.jar Scanner —getters g.test /classes  -> switch on null in Scanner) 

Throwing a RuntimeException now.

> 
> - include/exclude options will not work for modules - Scanner.java combines -i and -im and verifies classname against the filter.
> It means that for -im java.base class will be checked the same way as for -i java.base. You can reproduce the problem by modifying test in the MainTest:
> "-i", "java.util" -> "-im", “java.base” should give the same test’s result.

Does it mean that I am incorrectly using JCov API? Could you point out where I use it incorrectly? I have assumed that Utils.Pattern, if created with the third parameter set to true, would filter based on module name. This is what I seemed to find in JCov code. Could you point me where I am incorrect?


Shura

> 
> 
> Thank you.
> 
> —Alexey
> 
> 
>> On May 8, 2018, at 5:24 PM, Alexandre (Shura) Iline <alexandre.iline at oracle.com> wrote:
>> 
>> New version:
>> http://cr.openjdk.java.net/~shurailine/7902161/webrev.01/
>> 
>> Shura
>> 
>>> On May 8, 2018, at 2:03 PM, Alexandre (Shura) Iline <alexandre.iline at oracle.com> wrote:
>>> 
>>> 
>>> 
>>>> On May 8, 2018, at 11:54 AM, Alexey Fedorchenko <alexey.fedorchenko at oracle.com> wrote:
>>>> 
>>>> Hello! 
>>>> 
>>>> Thank you for the contribution.
>>>> 
>>>> Could you, please, address these few things to look deeper in the plugin functionality:
>>>> - readme file can help plugin users to understand how to run the plugin and what to expect (for an example what minimal JDK to use because it differs from the JCov required level)
>>> 
>>> Absolutely! The reason I did not do it right away is because I thought there could be comments on the command line syntax or functionality. Since you seem to be OK with both, I will proceed cresting the readme.
>>> 
>>>> - build.xml has “dest” “testng.jar” properties, it looks like the patch missed the default properties file.
>>> 
>>> You meant “dist”. Y, sorry, fixed. The directory was just created but not used. Leftovers of pervious version.
>>> 
>>> “testing.jar” I think I am processing correctly. You can build without worrying about testng existence, but if you wanna run test, you need to provide it. I will make sure to mention that in readme.
>>> 
>>> 
>>>> - default target in the build.xml is “dist” but the target does not exist in the build.xml
>>> 
>>> Oh!. Oversight - thanks!
>>> 
>>>> - what ASM is required by the plugin?
>>> 
>>>> Current requirement (asm.tree.checksum) is set to asm-tree-6.0 by the OS JCov build, but Scanner class uses clazz.methods list as the list of the MethodNodes (the change from the 6.1).
>>> 
>>> Let me look into that.
>>> 
>>> Thanks!
>>> 
>>> Shura
>>> 
>>> 
>>>> 
>>>> Thank you.
>>>> 
>>>> 
>>>> —Alexey
>>>> 
>>>> 
>>>>> On May 3, 2018, at 3:20 PM, Alexandre (Shura) Iline <alexandre.iline at oracle.com> wrote:
>>>>> 
>>>>> Hi.
>>>>> 
>>>>> Could you please take a look on this suggested addition to JCov.
>>>>> 
>>>>> It is a tool which could be used to discover “simple” methods in the code to later be used for filtering of coverage data, such as for ANC code. 
>>>>> 
>>>>> Functionality: While writing, I did make a number of design choices which may or may not be the most practical ones, so I would welcome any sort of feedback.
>>>>> 
>>>>> Testing: There are _some_ tests - not a complete coverage, which would require testing many combinations of bytecode operations. All tests are true “unit” tests with the exception of MainTest. Tests are executable from ant build script.
>>>>> 
>>>>> Documentation: There is _some_ javadoc here and there and there is a usage information - altogether not enough to consider the tool to be completely documented. But … it’s an open-source and the code is right next to the documentation … and the tool is for people with dev background, so I think we can start with what t is there already and add more later. Do let me know if something is incorrect, unclear or confusing - I will have it fixed.
>>>>> 
>>>>> Webrev: http://cr.openjdk.java.net/~shurailine/7902161/webrev.00
>>>>> 
>>>>> Thank you.
>>>>> 
>>>>> Shura
>>>>> 
>>>> 
>>> 
>> 
>