Split and Organize Checkstyle inputs by Test #3891

Closed
rnveach opened this Issue Mar 1, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@rnveach
Member

rnveach commented Mar 1, 2017

Taken from #3887 (comment)

It makes it hard to review changes to one Check when multiple Checks are using the same input file and users modify that one file. It makes sense, imo, to add new stuff to an existing input file for simple fixes instead of always creating a new one.

We already restrict inputs to only be used by tests in a single folder. We should take this further and restrict inputs to a single test so no 2 tests use the same input file.

Each PR should be one single Test class and it's inputs for quicker review, unless an admin has specified otherwise.

We should start by renaming all inputs to Input[SimpleTestName].* ([]s are just for separation and should not be in final file name), that are used by a test. If test is named ParenPadCheckTest, simple name would be ParenPad and input would start with InputParenPad. Any text can be appended to the end to make the input file name unique among others for the same test.

Each test's inputs should be alone in their own directory. If Test was in directory .../checks/design then inputs should be located in .../checks/design/[SimpleTestName] ([]s are just for separation and should not be in final file name). Folder names should be in all lowercase.
Files should be moved to their new locations unless they are used by other checks. If that is the case, then the files should be copied over until all tests have their inputs separated. No input should be left without a test.

We expect no changes to the test's expected violation positions (line or column number). If that does happen because of any of the requested changes, then some of the changes need to be undone.
If violation changes because of class rename, then class name and file name must be left unchanged but package rename and moving to new folder can still be done.
These type of problems will be handled in the final commits.

Once all inputs are renamed, we should see if we can use sevntu's Test to verify all current and future inputs are named after their tests and we have no isolated inputs. https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/AllChecksTest.java#L111

@rnveach rnveach added the easy label Mar 1, 2017

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Mar 1, 2017

Contributor

@rnveach

We should start by renaming all inputs to Input[SimpleTestName].* that are used by a test. If test is named ParenPadCheckTest, simple name would be ParenPad. Any text can be appended to the end to make the input file name unique among others for the same test.

If 2 checks use the exact same file, then that file should be duplicated and each one named after the test as stated above.

Very good ideas as for me. It is really annoying to read a huge input file for a test especially when there is a mixed content (two check's options use the same file, etc).

By the way, if we have individual input file for each test, we can add //violation comment to each file to make the test input easy to understand and quick to read. In addition, lets check and update input files for ITs too.

Contributor

MEZk commented Mar 1, 2017

@rnveach

We should start by renaming all inputs to Input[SimpleTestName].* that are used by a test. If test is named ParenPadCheckTest, simple name would be ParenPad. Any text can be appended to the end to make the input file name unique among others for the same test.

If 2 checks use the exact same file, then that file should be duplicated and each one named after the test as stated above.

Very good ideas as for me. It is really annoying to read a huge input file for a test especially when there is a mixed content (two check's options use the same file, etc).

By the way, if we have individual input file for each test, we can add //violation comment to each file to make the test input easy to understand and quick to read. In addition, lets check and update input files for ITs too.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 3, 2017

Member

Small update to issue to describe that []s are just for easy name separation and shouldn't be in final file name.
Small change that all inputs should be in their own directory separated from every other test in same package.

@MEZk

lets check and update input files for ITs too.

definately.

By the way, if we have individual input file for each test, we can add //violation comment to each file to make the test input easy to understand and quick to read.

Originally I thought this was ok, but the downside to this is we won't have the same input file for different runs with different properties of the check. We could see something by turning properties on/off on same file.
My only counter ideas are either placing a special violation message distinguishing which test is causing the violation or make on/off files special and kept in-sync by CI.

Member

rnveach commented Mar 3, 2017

Small update to issue to describe that []s are just for easy name separation and shouldn't be in final file name.
Small change that all inputs should be in their own directory separated from every other test in same package.

@MEZk

lets check and update input files for ITs too.

definately.

By the way, if we have individual input file for each test, we can add //violation comment to each file to make the test input easy to understand and quick to read.

Originally I thought this was ok, but the downside to this is we won't have the same input file for different runs with different properties of the check. We could see something by turning properties on/off on same file.
My only counter ideas are either placing a special violation message distinguishing which test is causing the violation or make on/off files special and kept in-sync by CI.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 3, 2017

Member

lets do split inputs for now only.

upgrade for UTs to describe configuration in comments and have strict "//violation" location is good approach that we already practicing, but lets do this as separate step - separate issue.

Member

romani commented Mar 3, 2017

lets do split inputs for now only.

upgrade for UTs to describe configuration in comments and have strict "//violation" location is good approach that we already practicing, but lets do this as separate step - separate issue.

izishared added a commit to izishared/checkstyle that referenced this issue Mar 31, 2017

izishared added a commit to izishared/checkstyle that referenced this issue Mar 31, 2017

romani added a commit that referenced this issue Sep 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 8, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 9, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 9, 2017

rnveach added a commit that referenced this issue Sep 10, 2017

rnveach added a commit that referenced this issue Sep 10, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 10, 2017

romani added a commit that referenced this issue Sep 12, 2017

romani added a commit that referenced this issue Sep 12, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 12, 2017

Member

Any file organization that is left over is split into the 2 issues above.

All that is left in this issue is to create the UT that enforces our naming scheme and fix any other remaining files we missed.

Member

rnveach commented Sep 12, 2017

Any file organization that is left over is split into the 2 issues above.

All that is left in this issue is to create the UT that enforces our naming scheme and fix any other remaining files we missed.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 13, 2017

Member

@rnveach , I recommend to close this issue and create separate issue for UT.
issues with long history are scary.

Member

romani commented Sep 13, 2017

@rnveach , I recommend to close this issue and create separate issue for UT.
issues with long history are scary.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 17, 2017

Member

@romani after #5120 is merged, I only have 1 PR left which is the UT and the final fixes.
Then this issue can be closed.

Member

rnveach commented Sep 17, 2017

@romani after #5120 is merged, I only have 1 PR left which is the UT and the final fixes.
Then this issue can be closed.

romani added a commit that referenced this issue Sep 21, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 22, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 22, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 22, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 22, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 22, 2017

romani added a commit that referenced this issue Sep 23, 2017

@romani romani added this to the 8.3 milestone Sep 23, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 23, 2017

Member

all fixes are merged, if smth is left, should be separate issue(s)

Member

romani commented Sep 23, 2017

all fixes are merged, if smth is left, should be separate issue(s)

@romani romani closed this Sep 23, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Nov 1, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jun 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment