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

[Jonathan Gibbons]

On 5/9/18 5:24 PM, Alexey Fedorchenko 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.
Although opinions differ, author info in files is generally discouraged.
Authorship is generally more accurately tracked through the SCM history.

>
> - 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 “
> 	(btw do we need 2 lines here? The first command will not succeed.)
>
> - include/exclude options (long and short names) differs in readme and in command line.
>
> - 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
>
> - 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)
>
> - testng.jar property is not enough to run tests, users will need to add jcommander to the classpath (“test” target is not executable).
>
> - command without uri schema will give NullPointerException
> (java -jar jcov.jar:SimpleMethods.jar Scanner —getters g.test /classes  -> switch on null in Scanner)
>
> - 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.
Please consider following the guidelines in JEP 293, unless there is a 
compelling reason to do otherwise.
http://openjdk.java.net/jeps/293

Options should either be "long-form" ... a short series of words 
prefixed by '--' and separated by '-',  or "short-form", a '-' followed 
by a single letter. Arguments may follow the name of a option separated 
by '=' or whitespace.

     --this-is-an-option
     -t
     --this-option-has-a-value=3
     --this-option-has-a-value=3
     -u 3


>
>
> 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/ <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 <mailto:alexandre.iline at oracle.com>> wrote:
>>>
>>>
>>>
>>>> On May 8, 2018, at 11:54 AM, Alexey Fedorchenko <alexey.fedorchenko at oracle.com <mailto: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 <mailto: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 <http://cr.openjdk.java.net/~shurailine/7902161/webrev.00>
>>>>>
>>>>> Thank you.
>>>>>
>>>>> Shura
>>>>>