Issue #6981: Renamed Complete IT Regression codebase & Added new Pattern check for input file naming convention #14595
Conversation
MANISH-K-07
force-pushed
the
renameIT
branch
2 times, most recently
from
940ee42
to
7dd1fa6
Compare
MANISH-K-07
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
My suggestion is to get existing PRs across the finish line, and not keep opening new ones and spamming maintainers. https://github.com/checkstyle/checkstyle/pulls/MANISH-K-07 We generally recommend for contributors to have only three PRs open at a time. |
Sorry about that, I will focus on getting my existing PRs to success. Rgarding the changes of this current PR, I have added the Test to accept a new pattern which was proposed as a permanent fix in discussions of Issue #6981 and following up with @romani 's suggestion at #6207 (comment)
This PR was a requirement inorder to fix the mentioned Issue #6981 |
| final String check = ALLOWED_DIRECTORY_AND_CHECKS.get(checkDir.toFile().getName()); | ||
|
|
||
| try (DirectoryStream<Path> inputPaths = Files.newDirectoryStream(checkDir)) { | ||
| for (Path inputPath : inputPaths) { | ||
| final String filename = inputPath.toFile().getName(); | ||
| if (filename.endsWith("java")) { | ||
| final Matcher matcher = pattern.matcher(filename); | ||
| if (filename.contains("SuppressionXpathRegression") && filename.endsWith("java")) { |
There was a problem hiding this comment.
I am not seeing why we have this check for SuppressionXpathRegression explicitly, why we have 2 chunks of duplicated code, and how this somehow only applies to ExecutableStatementCheck when there are still other checks to do.
Also the first post says this is the first part of this issue, but I am not seeing explanation on what is the other parts.
There was a problem hiding this comment.
@rnveach , From your #6981 (comment) :
Our regression area in IT should be fixed to match our pattern in Test folder.
Google and Sun area should atleast start with Input, but their folder structure follows a different pattern.
The input files of Google and sun area already start with word "Input".
The regression area however is too liberal.
Also the first post says this is the first part of this issue, but I am not seeing explanation on what is the other parts.
Approach :
- Rename all input files of regression area to match
InputXpath{CheckName}Xxxx.java - Organize the input files folder-wise like we did in our test folder (second part of issue as mentioned at comment)
- Organize the regression tests folder-wise to match the test folder pattern.
- Rename google and sun area files to be more specific of functionality (similar to my Issue 14436)
This can all be done in a single PR but it would complicate debugging and also the review process
I am not seeing why we have this check for SuppressionXpathRegression explicitly, why we have 2 chunks of duplicated code
At present, XpathRegressionTest is designed to fail if input file doesn't follow pattern SuppressionXpathRegressionXxxx.java.
All our files are named that way, so we need to allow them until this issue is fixed. So, I designed the code (suffixed "temp") to surpass if it finds a name containig "SuppressionXpathRegression"
From your discussion at #6207 (comment) and #6207 (comment) :
If we are talking about in the IT tier, I ask that we use a different pattern then Test.
I recommend to add such pattern in validation and allow two patterns (with input prefix and existing ) while work in progress.
The duplicate code I have designed to allow the new pattern InputXpath{CheckName}Xxxx.java while I work on fixing all the files. Once this is done, we can remove the previous chunk which is extra and update #6207 description intimating new contributors to follow new naming pattern for regression files.
and how this somehow only applies to ExecutableStatementCheck when there are still other checks to do.
The pattern applies to all of the regression area..
ExecutableStatementCheck was just my way of showing proof that my design works for new pattern.
The test was actually supposed to fail for the changes that I have made to executablestatementcount inputs, but because of the new chunk of code, it passes as I expected.
Also, I needed your and @romani 's approval for the new pattern InputXpath{CheckName}Xxxx.java
Once you are okay with this, I will send further PRs with all other checks
There was a problem hiding this comment.
I needed your ... approval for the new pattern InputXpath{CheckName}Xxxx.java
I have no issue here.
@romani gave his approval, so I don't think the pattern has any approval, but I am going to reassign him because of my next post.
Approach
Rename all input files of regression area to match InputXpath{CheckName}Xxxx.java
Tied into the next post, but this more an overall approach, including multiple issues and future goals. I am looking at only this issue, which is only file naming convention, and more importantly this PR to understand how this will fit into the overall issue. This PR is only a small part of your first bullet and it wasn't helping me see your goal for this.
The duplicate code I have designed to allow the new pattern InputXpath{CheckName}Xxxx.java while I work on fixing all the files.
ExecutableStatementCheck was just my way of showing proof that my design works for new pattern.
Now I understand what I was missing.
This PR doesn't enforce one pattern or another. It allows both the new and old pattern and users can "choose" what they want. The problem with this is that this goes against how we have done other similar issues. Normally we create suppressions or a list of things to do. That is how we know what is and isn't done, when an issue is fully complete, and people can pick from the list on what to work on a small part. We can't tell this with how you have it now. Is this 1 of 10, or 1 of a million? New contributors can accidentally copy the old pattern which will increase the list. There are so many files to do and limited time before GSoC, I doubt this can be completed quickly. I don't know your past work, so I am not basing this on any judgement of you but we take time to review and agree.
Unless someone interjects, we need to change this to be a enforcement with a list of to be completed. I wouldn't make this a exact file match but a parent folder match, which is the check name. So completing a folder completes a check. Anything in that folder can be whatever it wants if it is part of that list, but anything outside the list must be InputXpath.... We are then good to completely swap old enforcement with new as it will be fixed as your list is worked on. The list must reference the issue to work on (until https://....) and complete it. I recommend to make this a new issue, we can mark it as easy and maybe others will join you.
For this PR, I expect 2 commits. 1 commit of a full list, and the other with your example of ExecutableStatementCheck. This is a small enough subset as an example on how to do future PRs.
====
As a completely separate thought to improve your code, you probably could do away with all your changes and change:
final Pattern pattern = Pattern.compile("^SuppressionXpathRegression(.+)\\.java$");
to the following instead:
final Pattern pattern = Pattern.compile("^(?:SuppressionXpathRegression|InputXpath)(.+)\\.java$");
But I am thinking of a different path now so we can't use it.
There was a problem hiding this comment.
The easiest way to handle this is to have some file with all the "bad" file names in it. We could create a hacked version of this method to generate this file, then read it in when this validation runs and filter out failures by the contents of the file.
There was a problem hiding this comment.
@nrmancuso , do you mean something like a temporary regex check/test?
Could you please elaborate on why we would need this? I see that the issue is going to be very short-lived. Then we can simply revert back to our original method with changed pattern?
Or please correct me if am I going in the wrong direction.. :)
There was a problem hiding this comment.
We need to make sure that other maintainers are on board, but my proposal is this:
- Hack this method locally to dump full filenames of the "bad" input files (with path) to a file
- Update this method to read the file in as a collection of files to ignore
- Update this method to only check for the new pattern, and filter failures by the suppression file
Then, as we rename input files, we can remove them from the generated list. This will prevent anyone from adding new files with bad naming.
This comment was marked as outdated.
This comment was marked as outdated.
Unless we are not talking about the same thing, it should, but you will run into issues with complexity if you put too many conditions in that one if. Example: and |
| final String check = ALLOWED_DIRECTORY_AND_CHECKS.get(checkDir.toFile().getName()); | ||
|
|
||
| try (DirectoryStream<Path> inputPaths = Files.newDirectoryStream(checkDir)) { | ||
| for (Path inputPath : inputPaths) { | ||
| final String filename = inputPath.toFile().getName(); | ||
| if (filename.endsWith("java")) { | ||
| final Matcher matcher = pattern.matcher(filename); | ||
| if (filename.contains("SuppressionXpathRegression") && filename.endsWith("java")) { |
There was a problem hiding this comment.
Unless we are not talking about the same thing, it should, but you will run into issues with complexity if you put too many conditions in that one if.
Yes, I believe we are talking about the same thing :)
Each if only had one condition as shown below. The following wasn't allowed though. I had failing build on local
It was actually observed here..
At first, I roughly used :
if (filename.endsWith("java") {
if (filename.contains("SuppressionXpathRegression")) {
// code block
}
}
An error was reported saying the ifs could be combined, which is obvious
There was a problem hiding this comment.
The line you are commenting on is a collapsed nested if, which you said in the first post wasn't acceptable. :)
https://jsparrow.github.io/rules/collapse-if-statements.html#simple-collapse
There was a problem hiding this comment.
Yep, it is good that we check where ifs can be combined. ;)
However, I remember that day vigorously browsing through the codebase for the class/method which checks this.
Still am infact, but I somehow don't seem to find it....
MANISH-K-07
force-pushed
the
renameIT
branch
3 times, most recently
from
59e489f
to
5db2585
Compare
|
To all the maintainers,
We still didn't agree on one proposal to tackle this and in this gap, I did my best to help.... From #14595 (comment) :
@romani , @nrmancuso One more drawback of leaving this as an open issue is that few old and few new pattern named files would be confusing during GSoC period. I wanted to avoid this.
I agree there were a lot of files to modify ( 997 Files changed !! 3 more and I would have hit 1K 馃槀 ). It took me a lot of time to debug too. However I Managed to complete everything with a successful build in a day and two sleepless nights 馃槄
Hope I have grabbed all your good attention now and plan on doing the same going further too ;) |
I have completed the first step in plan of action..
Also, with your approval, I would like to create a two new Issues :
As a footnote, I now pretty much have gone through about 80-90 % of our codebase :) |
MANISH-K-07
changed the title
|
@MANISH-K-07 I asked for a suppression list at #14595 (comment) and only 1 check to be done , not for all the files to be done in 1 go. Another admin was the one who pinged me what happened here and. I don't believe we can accept this as this PR is too big to review in one go. We prefer smaller PRs as they are easier to review by us. |
|
I do not know how to review this PR, github is not able to render it. I am supporting comment above. @MANISH-K-07 , please never create massive PRs it is always a problem for both sides, and it almost always problematic with merge and leakage of side effects. |
There was a problem hiding this comment.
We prefer smaller PRs as they are easier to review by us.
@MANISH-K-07 , please never create massive PRs it is always a problem for both sides
Sorry, I'll make sure to keep these in mind 馃毇
can you split it two commits ? one commit is for functional changes of validation and second commit is just massive renaming of all.
Sure, 993 out of 997 files changed are it regression inputs and tests.
Only 4 following files were changed during debug and I will split these into minor commit
| @@ -85,7 +85,7 @@ | |||
| <suppress checks="OuterTypeFilename" | |||
| files="[\\/]package-info\.java"/> | |||
| <suppress checks="OuterTypeFilename" | |||
| files="src[\\/]it[\\/]resources[\\/].*[\\/]SuppressionXpathRegressionOuterTypeFilename.*\.java"/> | |||
| files="src[\\/]it[\\/]resources[\\/].*[\\/]InputXpathOuterTypeFilename.*\.java"/> | |||
There was a problem hiding this comment.
checkstyle-resource-suppressionswhere SuppressionXpathRegressionOuterTypeFilename was being suppressed for OuterTypeFilename check. Had to change the suppression here to new naming.
| @@ -8,7 +8,7 @@ | |||
|
|
|||
| package com.puppycrawl.tools.checkstyle.checks.blocks.avoidnestedblocks; | |||
|
|
|||
| import static org.checkstyle.suppressionxpathfilter.interfaceistype.SuppressionXpathRegressionInterfaceIsType1.a; | |||
| import static org.checkstyle.suppressionxpathfilter.interfaceistype.InputXpathInterfaceIsType1.a; | |||
|
|
|||
| // xdoc section -- start | |||
| public class Example1 { | |||
There was a problem hiding this comment.
- avoidnestedblocks/Example1.java was importing a file from it folder. Had to change import with new name.
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| package com.puppycrawl.tools.checkstyle.checks.blocks.avoidnestedblocks; | |||
|
|
|||
| import static org.checkstyle.suppressionxpathfilter.interfaceistype.SuppressionXpathRegressionInterfaceIsType1.a; | |||
| import static org.checkstyle.suppressionxpathfilter.interfaceistype.InputXpathInterfaceIsType1.a; | |||
|
|
|||
| // xdoc section -- start | |||
| public class Example2 { | |||
There was a problem hiding this comment.
- Same as above. Had to change import with new name in avoidnestedblocks/Example2.java
| @@ -264,7 +264,7 @@ public void validateInputFiles() throws Exception { | |||
| } | |||
|
|
|||
| private static void validateInputDirectory(Path checkDir) throws IOException { | |||
| final Pattern pattern = Pattern.compile("^SuppressionXpathRegression(.+)\\.java$"); | |||
| final Pattern pattern = Pattern.compile("^InputXpath(.+)\\.java$"); | |||
| final String check = ALLOWED_DIRECTORY_AND_CHECKS.get(checkDir.toFile().getName()); | |||
|
|
|||
There was a problem hiding this comment.
- New pattern has been updated here to be validated
|
I am not sure why @romani wants 2 commits if he intends all 1k files to stay here (which is what my impression is). I cannot review a 1k file change in any timely matter. I stand by my original statement that we just need the test, suppression, and the 1 example in this PR (2 commits). I would think this would ultimately need to be split among 50 PRs (20 files each) at the minimum. I originally felt that @MANISH-K-07 had better issues to work on then spending so much time on this one. |
MANISH-K-07
force-pushed
the
renameIT
branch
2 times, most recently
from
8840b96
to
690fe2e
Compare
@rnveach , I understand this is not possible to review and have made a mistake. If there isn't any option and this PR is a hard block, then I will revert and proceed with suppression list just like you suggested.
I actually maintain multiple clones of the project in different directories (atleast 3-4 running on multiple powershells). This way, the time that goes by during mvn validations (usually about an hour and a half or more for all validations), I utilize this time to work parallely on a different issue. Also, this is actually how I manage to spend max time on the project despite university exams and research publications. |
|
@rnveach , please review first commit. Let's agree on it. Second commit I can try to review on my next keyboard time, not sure when, but weekend is close. Can we can merge just renaming before validation (first commit)? |
|
A new Xpath regression test has been introduced as a result of merging #14639 . |
This is not because of changes in this PR. It was a leak in master and fixed by my commit #14658 . However, #14639 needs to be applied here, Proof that new pattern is being validated |
|
Maintainers are ok to continue this PR |
@romani , I didn't understand. Why close the PR then? |
|
@MANISH-K-07 The word "not" was missing from his text. Like I mentioned at #14595 (comment) and #14595 (comment) , no one can review a 1k change PR. We are not saying you should abandon your work. We just need it split up like I mentioned previously.
|
|
Ohh Okay.. |
|
Yes, but be sure it is 2 commits. We expect full suppression list in 1 commit, and then 1 item removed from suppression list in another commit with the test fixes. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@rnveach , from what I see, we will have to validate using |
|
It is sort of similar to |
Closes Issue #6981
:)
Updates in PR :
Complete IT Regression codebase ( 997 Files Changed )of our project has been renamed in this PR to follow newly discussed pattern -->
InputXpath{CheckName}Xxxx.javaXpathRegressionTest has been updated to check for the above mention new pattern.
---------------------------------------------------------------------------------------------------------------
<-- OUTDATED -->
Response to #6207 (comment) :
For Detailed Explanation : #14595 (comment)
I have added a new pattern check that verifies file name to comply with naming convention
InputXpath{CheckName}Xxxx.javaXpathRegressionTest now tests the input file names to be either-
This can be removed after final fix of the mentioned issue.
Unrelated : I just found out that checkstyle doesn't allow collapsible nested ifs 馃槄