Issue #79: Processing UT changes to get possible property values#85
Issue #79: Processing UT changes to get possible property values#85Luolc wants to merge 1 commit intocheckstyle:masterfrom
Conversation
| // Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| //////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| package com.github.checkstyle.regression.module; |
There was a problem hiding this comment.
How about we put custom checks in a new package?
Something like customcheck.
| import com.puppycrawl.tools.checkstyle.api.TokenTypes; | ||
|
|
||
| /** | ||
| * The custom check which processes the unit test class of a checkstyle module. |
There was a problem hiding this comment.
Yes we 'process' the check, but it doesn't specify what it grabs from said processing.
First sentence should be a quick summary, 2nd+ sentences should talk about details.
| import com.puppycrawl.tools.checkstyle.api.TokenTypes; | ||
|
|
||
| /** | ||
| * The custom check which processes the unit test class of a checkstyle module. |
There was a problem hiding this comment.
Add extra blank line between summary and details.
| * This check would walk through the {@code @Test} annotation, find variable definition | ||
| * like {@code final DefaultConfiguration checkConfig = createModuleConfig(FooCheck.class)} | ||
| * and grab the property info from {@link DefaultConfiguration#addAttribute(String, String)} | ||
| * method call. |
There was a problem hiding this comment.
So we won't support tests where checkConfig is defined as a field, instead of a local variable?
It is not possible to support both?
If so, we need changes in Checkstyle as there are loads of different ways tests are made and they must be organized.
Please make a new issue in checkstyle and let's get @romani 's approval.
If we are going this way, please update Javadoc with example of how test classes should be defined.
There was a problem hiding this comment.
So we won't support tests where checkConfig is defined as a field, instead of a local variable?
It is not possible to support both?
I think it could be possible. I didn't realize the config could be a field before. I would have a try on this.
There was a problem hiding this comment.
@Luolc Please use your check and scan Checkstyle's repo. Any true module tests where your check finds nothing or not enough is an area you need to look into.
| final List<File> processedFiles = Collections.singletonList(new File(path)); | ||
| checker.process(processedFiles); | ||
|
|
||
| final UnitTestProcessorCheck check = getCheckInstance(checker); |
There was a problem hiding this comment.
Truthfully we don't need Check's instance. We just need unitTestToPropertiesMap.
Why don't we make field static and create a getter for it? Than we can just do UnitTestProcessorCheck.getUnitTestToPropertiesMap instead of doing reflection hacks.
There was a problem hiding this comment.
Changed to static method now.
| import com.puppycrawl.tools.checkstyle.api.TokenTypes; | ||
| import com.puppycrawl.tools.checkstyle.utils.CommonUtils; | ||
|
|
||
| public class ReturnCountCheckTest extends AbstractModuleTestSupport { |
There was a problem hiding this comment.
This confused me at first as I was expecting a test file for UnitTestProcessorCheck and was wondering where ReturnCount came from.
Does this need to be ReturnCount? Can we turn it into some dummy name like InputModuleExampleTest?
There was a problem hiding this comment.
Done. This class is a copy of ReturnCountCheckTest from checkstyle repo. I didn't change its name at first.
| * @throws CheckstyleException failure when running the check | ||
| * @throws ReflectiveOperationException failure of reflection on checker and tree walker | ||
| */ | ||
| public static Map<String, Set<ModuleInfo.Property>> process(String path) |
There was a problem hiding this comment.
This class has 2 functions, executing a custom check and getting the results for the specific check.
I assume we may need to write another custom check for #6 .
If you agree, I think we should rename this class to something more generic, have it take the module class as input, and return nothing as everything will be retrieved through static getters.
| // Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| //////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| package com.github.checkstyle.regression.module; |
There was a problem hiding this comment.
This should move with the custom check too.
config/pmd.xml
Outdated
| <rule ref="rulesets/java/comments.xml/CommentRequired" /> | ||
| <rule ref="rulesets/java/comments.xml/CommentRequired"> | ||
| <properties> | ||
| <property name="publicMethodCommentRequirement" value="Ignored"/> |
There was a problem hiding this comment.
Please explain this change.
pom.xml
Outdated
| </regex> | ||
| <regex> | ||
| <pattern>com.github.checkstyle.regression.module.UnitTestProcessorCheck</pattern> | ||
| <branchRate>76</branchRate> |
There was a problem hiding this comment.
While you are making changes, please post this file's code coverage report.
f66cd92 to
9b01307
Compare
|
@Luolc Remember to ping me when you have made fixes and reply as |
| <rule ref="rulesets/java/comments.xml/CommentRequired" /> | ||
| <rule ref="rulesets/java/comments.xml/CommentRequired"> | ||
| <properties> | ||
| <property name="violationSuppressXPath" value="//Annotation/MarkerAnnotation//Name[@Image='Override']"/> |
There was a problem hiding this comment.
Please explain this change.
There was a problem hiding this comment.
Without this pmd would force us to add javadoc on @Override method. I copied this settings from checkstyle repo.
There was a problem hiding this comment.
Ok, thanks.
This can be ignored.
pom.xml
Outdated
| </regex> | ||
| <regex> | ||
| <pattern>com.github.checkstyle.regression.module.UnitTestProcessorCheck</pattern> | ||
| <branchRate>76</branchRate> |
rnveach
left a comment
There was a problem hiding this comment.
re-verifying items that are now shown as hidden.
| } | ||
|
|
||
| /** | ||
| * Processes the file with the given path using the given custom check. |
There was a problem hiding this comment.
Update documentation that check needs a public/static way to retrieve it's own results using this method.
| * <p>This check would walk through the {@code @Test} annotation, find variable definition | ||
| * like {@code final DefaultConfiguration checkConfig = createModuleConfig(FooCheck.class)} | ||
| * and grab the property info from {@link DefaultConfiguration#addAttribute(String, String)} | ||
| * method call.</p> |
There was a problem hiding this comment.
If we are going this way, please update Javadoc with example of how test classes should be defined.
Not done.
| final DetailAST methodDef = ast.getParent().getParent(); | ||
| final DetailAST methodBlock = methodDef.findFirstToken(TokenTypes.SLIST); | ||
| final Optional<String> configVariableName = | ||
| getModuleConfigVariableName(methodBlock); |
There was a problem hiding this comment.
So we won't support tests where checkConfig is defined as a field, instead of a local variable?
It is not possible to support both?
I don't see an update or any tests for this.
|
@rnveach I scan and output all the property info from UTs, and I have to check them manually whether the processor missed anything or produce anything wrong. I am working on the config as class field and the enum property value, but I am still not very confident that they are absolutely good now. I added some reply and others are still working. Checking the result of all checkstyle module UTs may still take a lot of time I think. |
I would try automating it somehow and not worry about manually checking every single digit. For example, I said if you run your check does any module file not produce any results? I already gave you some examples of other areas that your check doesn't cover, so we can go with that for now. Those should be the most popular ways to code the tests. |
|
@Luolc ping |
|
@rnveach I made some updates and could also support value as:
The problems found currently: 2.The property value could be a variable:
3.The property value could be a variable:
4.Some UTs missed tests of some properties
example: https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/naming/ConstantNameCheckTest.java , no test for property 5.different way of class field module config There are many other UTs with class field config that is assign in 6.Some UTs don't use
we may need to collect names of all these kind of method. ------ |
Why regular expression? If it is in the form
I see
Let's split this off into another issue. The example you gave is pretty simple and the variable could be inlined. The check could also be modified to just track variable assignments and pickup the very basic uses.
Move this to another issue too. I think we should recognize
This must be an issue in Checkstyle.
I still think we should create an issue in Checkstyle to have them organize their UTs more.
Especially these 2. I don't see a reason right now why they can't be standardized.
Yes, we will need to do this for any issues we create. It would be better if we could atleast create an automated way to detect these instances and not rely on manually reading each one. |
Is it possible to automate finding of bad cases, even if it only finds like 80% of the cases? Automating the finding of bad cases would allow us to finish this specific issue faster and move work to fixing up Checkstyle's sources while we incorporate the data found here for more work we need to do in our logic. |
I don't know if we have smth like
OK. That makes sense.
Although
Maybe I need another custom check, or at least some kind of regex way for this. I would sum up the issues we need to create both here and in checkstyle repo and create them together sooner. |
yes, it would need to be in sevntu as it is just for us.
Yes, to get the correct path would would have to inspect the
I doubt it, but something like the custom check I mentioned in previous post to validate UTs that use I think it makes more and more sense we go this route and force checkstyle use the styling variants we approve. This will make things easier on our custom check for all the weird cases we could possibly see. |
|
@Luolc ping |
Issue #5104 in checkstyle was created for this. |
|
Point 1 is nearly fixed in Checkstyle checkstyle/checkstyle#5104 . This PR is on hold until the issues are fixed and uniformity is brought to Checkstyle. |
|
Custom check needs to be changed to gather information from multiple configurations in 1 test. |
|
New check is done and will soon be added to Checkstyle. Points 1, 2, 5, and 6 are done. All that is left is to fix this PR and merge it. |
#79
This PR includes a custom check and the utility class to run the check.
The check walk through all
@Testannotation to target the UT method, find the statement likefinal DefaultConfiguration fooModuleConfig = createModuleConfig(FooCheck.class), and get properties by detecting thefooModuleConfig.addAttributemethod call.If this is OK, then we could integrate this into our generator.