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

Split and Organize Checkstyle inputs for MainTests to 'main' folder #4588

Closed
romani opened this Issue Jul 2, 2017 · 17 comments

Comments

Projects
None yet
3 participants
@romani
Member

romani commented Jul 2, 2017

subtask of #3891

Move / Make a copy of all test input files to a separate folder "main" for MainTest.
Not any other XxxxxTest files should use that inputs.

There are a lot of other issues/commit that named as "Split and Organize Checkstyle inputs ..." , you can find example of what to do there.

https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle should be empty on completion.

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 9, 2017

Contributor

@romani
Could please give a more exact description of what should be done? Thanks

Contributor

Kietzmann commented Jul 9, 2017

@romani
Could please give a more exact description of what should be done? Thanks

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 9, 2017

Member

@Kietzmann Exact description can be found at #3891 (comment) .

Member

rnveach commented Jul 9, 2017

@Kietzmann Exact description can be found at #3891 (comment) .

Kietzmann pushed a commit to Kietzmann/checkstyle that referenced this issue Jul 10, 2017

Kietzmann pushed a commit to Kietzmann/checkstyle that referenced this issue Jul 10, 2017

Kietzmann pushed a commit to Kietzmann/checkstyle that referenced this issue Jul 10, 2017

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 10, 2017

Contributor

@romani
Do I need to move all resource files to /main package or only used by MainTest
*A lot of files in /checkstyle dir is not used by MainTest.java

Contributor

Kietzmann commented Jul 10, 2017

@romani
Do I need to move all resource files to /main package or only used by MainTest
*A lot of files in /checkstyle dir is not used by MainTest.java

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 10, 2017

Member

Only those by MainTest.
If there are still files leftover once done and they are not used by any other test, they should be removed.

Member

rnveach commented Jul 10, 2017

Only those by MainTest.
If there are still files leftover once done and they are not used by any other test, they should be removed.

Kietzmann pushed a commit to Kietzmann/checkstyle that referenced this issue Jul 11, 2017

Kietzmann pushed a commit to Kietzmann/checkstyle that referenced this issue Jul 12, 2017

Kietzmann pushed a commit to Kietzmann/checkstyle that referenced this issue Jul 12, 2017

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 12, 2017

Member

there still some files left in resource folder.
@Kietzmann , are they related to MainTest ?

Member

romani commented Jul 12, 2017

there still some files left in resource folder.
@Kietzmann , are they related to MainTest ?

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 12, 2017

Contributor

@romani
No, they're not. They're used by other tests

Contributor

Kietzmann commented Jul 12, 2017

@romani
No, they're not. They're used by other tests

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 12, 2017

Member

@Kietzmann , please provide list of test that use such files, we need to know what should be fixed.
This folder should be empty from files ideally.

Member

romani commented Jul 12, 2017

@Kietzmann , please provide list of test that use such files, we need to know what should be fixed.
This folder should be empty from files ideally.

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 12, 2017

Contributor

@romani
cache.tmp -> PropertyCacheFileTest
externalResource.tmp -> PropertyCacheFileTest
import-control_complete.xml -> FileTextTest, AllCheckTest
inputDefaultConfig -> AllChecksTest
inputImportControlOne.xml -> CheckerTest
inputImportControlOneRegExp.xml -> CheckerTest
InputMain -> CheckerTest, TreeWalkerTest
InputTreeWalkerHiddenComments -> TreeWalkerTest
java.header -> PropertyCacheFileTest, XdocsPagesTest
suppress_all.xml -> CheckerTest

I'll provide PR with removed unused files ASAP

Contributor

Kietzmann commented Jul 12, 2017

@romani
cache.tmp -> PropertyCacheFileTest
externalResource.tmp -> PropertyCacheFileTest
import-control_complete.xml -> FileTextTest, AllCheckTest
inputDefaultConfig -> AllChecksTest
inputImportControlOne.xml -> CheckerTest
inputImportControlOneRegExp.xml -> CheckerTest
InputMain -> CheckerTest, TreeWalkerTest
InputTreeWalkerHiddenComments -> TreeWalkerTest
java.header -> PropertyCacheFileTest, XdocsPagesTest
suppress_all.xml -> CheckerTest

I'll provide PR with removed unused files ASAP

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 12, 2017

Member

AllChecksTest and XdocsPagesTest is in "internal" package in test ... should be copied/moved to "internal" subfolder.
@rnveach , please confirm.

inputImportControlOne.xml -> CheckerTest, ImportControlCheckTest
inputImportControlOneRegExp.xml -> CheckerTest, ImportControlCheckTest

input files should be copied to "checks/input", looks like it was missed during #4437 .
@Kietzmann , please help us to finish cleanup in this folder.
in https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle should be left only inputs that belong to Tests files from https://github.com/checkstyle/checkstyle/tree/master/src/test/java/com/puppycrawl/tools/checkstyle

Member

romani commented Jul 12, 2017

AllChecksTest and XdocsPagesTest is in "internal" package in test ... should be copied/moved to "internal" subfolder.
@rnveach , please confirm.

inputImportControlOne.xml -> CheckerTest, ImportControlCheckTest
inputImportControlOneRegExp.xml -> CheckerTest, ImportControlCheckTest

input files should be copied to "checks/input", looks like it was missed during #4437 .
@Kietzmann , please help us to finish cleanup in this folder.
in https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle should be left only inputs that belong to Tests files from https://github.com/checkstyle/checkstyle/tree/master/src/test/java/com/puppycrawl/tools/checkstyle

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 12, 2017

Member

java.header -> XdocsPagesTest

This is not true. We use checkstyle's main java.header in config path, not a test's input.
See:

new File("config/java.header").getCanonicalPath());

Notice there is no getPath so it is relative to repository root.

@rnveach , please confirm.

I am fine with moving to internal folder for AllChecksTest.

Member

rnveach commented Jul 12, 2017

java.header -> XdocsPagesTest

This is not true. We use checkstyle's main java.header in config path, not a test's input.
See:

new File("config/java.header").getCanonicalPath());

Notice there is no getPath so it is relative to repository root.

@rnveach , please confirm.

I am fine with moving to internal folder for AllChecksTest.

romani added a commit that referenced this issue Jul 13, 2017

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 13, 2017

Contributor

@rnveach
Hi,
I'm receiving RegexpMultiline Error while checkstyle trying to check
return super.getPath("internal" + File.separator + filename);
Could you please tell me if I should suppress this check or modify my code?
Thanks

Contributor

Kietzmann commented Jul 13, 2017

@rnveach
Hi,
I'm receiving RegexpMultiline Error while checkstyle trying to check
return super.getPath("internal" + File.separator + filename);
Could you please tell me if I should suppress this check or modify my code?
Thanks

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 13, 2017

Member

What is a message of violation ?

Member

romani commented Jul 13, 2017

What is a message of violation ?

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 13, 2017

Contributor

Unnecessary consecutive lines [RegexpMultiline]

Contributor

Kietzmann commented Jul 13, 2017

Unnecessary consecutive lines [RegexpMultiline]

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 13, 2017

Member

@Kietzmann You should modify your code. It is complaining that you have 2 or more consecutive empty lines. IE: \n\n\n

Member

rnveach commented Jul 13, 2017

@Kietzmann You should modify your code. It is complaining that you have 2 or more consecutive empty lines. IE: \n\n\n

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 13, 2017

Contributor

Thanks

Contributor

Kietzmann commented Jul 13, 2017

Thanks

rnveach added a commit that referenced this issue Jul 13, 2017

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Jul 14, 2017

Contributor

What should I do with rest files?

Contributor

Kietzmann commented Jul 14, 2017

What should I do with rest files?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 14, 2017

Member

verified.
Lets keep that files there.
Issue is closed.

Member

romani commented Jul 14, 2017

verified.
Lets keep that files there.
Issue is closed.

@romani romani closed this Jul 14, 2017

@romani romani added this to the 8.1 milestone Jul 14, 2017

Kietzmann pushed a commit to Kietzmann/checkstyle that referenced this issue Jul 20, 2017

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