-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #79: Processing UT changes to get possible property values #85
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we put custom checks in a new package?
Something like customcheck
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add extra blank line between summary and details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should move with the custom check too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain this change.
pom.xml
Outdated
@@ -394,6 +394,11 @@ | |||
<lineRate>0</lineRate> | |||
</regex> | |||
<regex> | |||
<pattern>com.github.checkstyle.regression.module.UnitTestProcessorCheck</pattern> | |||
<branchRate>76</branchRate> | |||
<lineRate>92</lineRate> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you are making changes, please post this file's code coverage report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks.
This can be ignored.
pom.xml
Outdated
@@ -394,6 +394,11 @@ | |||
<lineRate>0</lineRate> | |||
</regex> | |||
<regex> | |||
<pattern>com.github.checkstyle.regression.module.UnitTestProcessorCheck</pattern> | |||
<branchRate>76</branchRate> | |||
<lineRate>92</lineRate> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@Test
annotation to target the UT method, find the statement likefinal DefaultConfiguration fooModuleConfig = createModuleConfig(FooCheck.class)
, and get properties by detecting thefooModuleConfig.addAttribute
method call.If this is OK, then we could integrate this into our generator.