Skip to content
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

Test inputs should be completely standalone. #4845

Open
rnveach opened this issue Jul 28, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@rnveach
Copy link
Member

commented Jul 28, 2017

Identified at #4844 (comment) ,

Inputs should be completely standalone and not rely on Checkstyle production code or test code.
It creates an usual coupling and requires changes in completely unrelated tests.

Most inputs that have references to checkstyle code: https://github.com/search?utf8=%E2%9C%93&q=path%3Asrc%2Ftest%2Fresources+filename%3AInput*.java+repo%3Acheckstyle%2Fcheckstyle+%22import+com.puppycrawl.tools.checkstyle%22&type=
There could be more as that was just a general search.

Imports have to be examined why they are used, and how best to remove them and still keep the tests original purpose intact.

@romani

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

I think we can do this by IllegalImport Check(or importorder). We could have special phase and config for this during build. It might be better to enforce a rule and generate list of suppression and remove items from suppression by each commit/PR

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2017

We should also extend this that we shouldn't import Inputs from other inputs unless really necessary.
Re-organizing inputs in #3891 hit an issue with Input15Extensions.
Example: https://github.com/search?utf8=%E2%9C%93&q=path%3Asrc%2Ftest%2Fresources+filename%3AInput*.java+repo%3Acheckstyle%2Fcheckstyle+%22import+com.puppycrawl.tools.checkstyle.checks.imports.unusedimports.Input15Extensions%22&type=

@pbludov

This comment has been minimized.

Copy link
Collaborator

commented Dec 30, 2017

This is another blocker for #5102. The sun packets are widely used in the import order checks.

@pbludov

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2018

For the ImportOrderCheck it is possible to add a number of specially arranged classes to test all the order cases. Something like that:

src
  test
     resources-importorder
         com
             pkg1
                Class1.java
                Class2.java
         org
             pkg2
                Class3.java
                Class4.java
         net
             pkg3
                Class5.java
                Class6.java
@romani

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

please do not be over crazy with this issue, it could result in huge update.
it is better to do what is required for java9 and that is all, it will be enough for next several years.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

This issue isn't even related to java 9, it is about checkstyle's own production code being used in inputs causing a needless link between the 2.

@pbludov

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2018

Thinking carefully, I realized that this was a bad idea. There is no need to create many packages within the CS only so that they can be referenced from the import check.

It's enough to move them to resources-noncompilable. How is this done in #5176.
As a free bonus, no smart IDE will re-arrange the import, as the IntelliJ Idea does it, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.