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 BaseCheckTestSupport into AbstractPathTestSupport, AbstractModuleTestSupport, and AbstractTreeTestSupport #4592

Closed
ps-sp opened this Issue Jul 3, 2017 · 20 comments

Comments

Projects
None yet
4 participants
@ps-sp
Collaborator

ps-sp commented Jul 3, 2017

Other test classes which aren't check tests also extend BaseCheckTestSupport . For example look at the following classes which extend BaseCheckTestSupport

AstTreeStringPrinterTest
JavadocParseTreeTest

In the future, there might be need for other non-check tests' classes to extend it too. It overall seems better to call it BaseTestSupport

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 5, 2017

Contributor

@romani I'll work on this.

Contributor

subkrish commented Jul 5, 2017

@romani I'll work on this.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 8, 2017

Member

@subkrish , @rnveach

I think here is big conceptual mis understanding .
BaseCheckTestSupport was designed for Checks and have a lot of methods that are for Checks only
image

Fact that other Test are using it it is their problem, they should not extend this class at all.
Example: CheckstyleAntTaskTest can have its own implementation of getPath and that is all that he use from BaseCheckTestSupport, extending is not appropriate.

AstTreeStringPrinterTest, JavadocParseTreeTest use only verifyAstXXXXX methods , so they could have that methods in their common parent, that is not BaseCheckTestSupport.

Filters\Suppressions do use verify methods as Tests are written in more functional way. It might tell us to rename BaseCheckTestSupport to BaseModuleTestSupport. But again CheckstyleAntTask, AstTreeStringPrinter, JavadocParseTree are not modules.

Member

romani commented Jul 8, 2017

@subkrish , @rnveach

I think here is big conceptual mis understanding .
BaseCheckTestSupport was designed for Checks and have a lot of methods that are for Checks only
image

Fact that other Test are using it it is their problem, they should not extend this class at all.
Example: CheckstyleAntTaskTest can have its own implementation of getPath and that is all that he use from BaseCheckTestSupport, extending is not appropriate.

AstTreeStringPrinterTest, JavadocParseTreeTest use only verifyAstXXXXX methods , so they could have that methods in their common parent, that is not BaseCheckTestSupport.

Filters\Suppressions do use verify methods as Tests are written in more functional way. It might tell us to rename BaseCheckTestSupport to BaseModuleTestSupport. But again CheckstyleAntTask, AstTreeStringPrinter, JavadocParseTree are not modules.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2017

Member

CheckstyleAntTaskTest can have its own implementation of getPath

Almost all tests should override getPath to support their own specific path. You can see this in checks where we take the original getPath and build upon it. We want everyone to put their inputs in their own directory. I dislike the idea of rewriting the full path in every test file. I like it when we extend and just append to the end.

rename BaseCheckTestSupport to BaseModuleTestSupport

I am fine with this.

CheckstyleAntTask, AstTreeStringPrinter, JavadocParseTree are not modules

I assumed the purpose of this class was to make a place for all common testing methods, hence why we we added verifyAst to it. I don't really see a big problem keeping it as it is. If we split this class into 2, than it will cause issues if we ever have a test that requires methods from both. I assume we will wait until this happens, unless someone can point to a place where it has already.

How about we make these 3 classes?

  1. AbstractModuleTestSupport - contains everything module run related. Extends AbstractPathTestSupport.
  2. AbstractTreeTestSupport - contains everything tree related. Extends AbstractPathTestSupport .
  3. AbstractPathTestSupport - contains only the bare needed getPath method. Extends nothing.

I still think the Abstract classes should be abstract to signal they are not to be used without extending.

AbstractPathTestSupport will have an abstract method getPackageLocation. All getPath methods will use that to identify the save location as inputs should be in the same directory structure anyways. All getPath methods should be final. Only AbstractPathTestSupport will have the only abstract methods. No other test supports will define their own abstract methods, unless we come upon something later.
Example:

protected final String getPath(String filename) throws IOException {
    return new File("src/test/resources/" + getPackageLocation() + "/" + filename)
        .getCanonicalPath();
}

I also feel we should make similar changes to the it section and keep them in-synch.

Member

rnveach commented Jul 8, 2017

CheckstyleAntTaskTest can have its own implementation of getPath

Almost all tests should override getPath to support their own specific path. You can see this in checks where we take the original getPath and build upon it. We want everyone to put their inputs in their own directory. I dislike the idea of rewriting the full path in every test file. I like it when we extend and just append to the end.

rename BaseCheckTestSupport to BaseModuleTestSupport

I am fine with this.

CheckstyleAntTask, AstTreeStringPrinter, JavadocParseTree are not modules

I assumed the purpose of this class was to make a place for all common testing methods, hence why we we added verifyAst to it. I don't really see a big problem keeping it as it is. If we split this class into 2, than it will cause issues if we ever have a test that requires methods from both. I assume we will wait until this happens, unless someone can point to a place where it has already.

How about we make these 3 classes?

  1. AbstractModuleTestSupport - contains everything module run related. Extends AbstractPathTestSupport.
  2. AbstractTreeTestSupport - contains everything tree related. Extends AbstractPathTestSupport .
  3. AbstractPathTestSupport - contains only the bare needed getPath method. Extends nothing.

I still think the Abstract classes should be abstract to signal they are not to be used without extending.

AbstractPathTestSupport will have an abstract method getPackageLocation. All getPath methods will use that to identify the save location as inputs should be in the same directory structure anyways. All getPath methods should be final. Only AbstractPathTestSupport will have the only abstract methods. No other test supports will define their own abstract methods, unless we come upon something later.
Example:

protected final String getPath(String filename) throws IOException {
    return new File("src/test/resources/" + getPackageLocation() + "/" + filename)
        .getCanonicalPath();
}

I also feel we should make similar changes to the it section and keep them in-synch.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 8, 2017

Member

I agree on this.

@subkrish , fyi - #4661 . And please think how to make that changes steps by step without huge PR.
we need to cut Ant first from hierarchy
then cut from hierarchy AstTreeStringPrinter, JavadocParseTree
third PR will be about renaming or changing BaseCheckTestSupport

Member

romani commented Jul 8, 2017

I agree on this.

@subkrish , fyi - #4661 . And please think how to make that changes steps by step without huge PR.
we need to cut Ant first from hierarchy
then cut from hierarchy AstTreeStringPrinter, JavadocParseTree
third PR will be about renaming or changing BaseCheckTestSupport

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2017

Member

After talking with @romani we added AbstractPathTestSupport for classes that don't fall under the other 2. It will only contain the bare needed 2 methods, getPackageLocation and getPath.
Ant and any other weird ones that don't fall under the other 2 should use it.

Member

rnveach commented Jul 8, 2017

After talking with @romani we added AbstractPathTestSupport for classes that don't fall under the other 2. It will only contain the bare needed 2 methods, getPackageLocation and getPath.
Ant and any other weird ones that don't fall under the other 2 should use it.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 8, 2017

Contributor

@romani @rnveach

Regarding first PR, CheckstyleAntTaskTest only uses getPath(). I think it can directly just extend AbstractPathTestSupport as you have suggested that it is not a module and also in the above comment.

Also 'module related' meaning all the 'checks' that extend from BaseCheckTestSupport right?

One more thing, I think AbstractPathTestSupport must also contain getNonCompilablePath() or will this be in AbstractModuleTestSupport?

Contributor

subkrish commented Jul 8, 2017

@romani @rnveach

Regarding first PR, CheckstyleAntTaskTest only uses getPath(). I think it can directly just extend AbstractPathTestSupport as you have suggested that it is not a module and also in the above comment.

Also 'module related' meaning all the 'checks' that extend from BaseCheckTestSupport right?

One more thing, I think AbstractPathTestSupport must also contain getNonCompilablePath() or will this be in AbstractModuleTestSupport?

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jul 9, 2017

Collaborator

Why can't we have an abstract class BaseTestSupport with constants like static final File TEST_DIR = new File(...) and abstract methods like getPath() and can also have all the other general methods in it. It can be extended by BaseCheckTestSupport (and other ...TestSupport classes if necessary). This also helps overcome the issue of some test requiring methods from multiple test support classes, since if a test actually has such requirement then that means
either
the method is more general than the particular ...TestSupport class it resides in and hence can be moved upward in the parent class.
or
the test doesn't belong to any of the genres of the existing support classes and requires a new ...TestSupport class which would extend the abstract BaseTestSupport class.

Collaborator

ps-sp commented Jul 9, 2017

Why can't we have an abstract class BaseTestSupport with constants like static final File TEST_DIR = new File(...) and abstract methods like getPath() and can also have all the other general methods in it. It can be extended by BaseCheckTestSupport (and other ...TestSupport classes if necessary). This also helps overcome the issue of some test requiring methods from multiple test support classes, since if a test actually has such requirement then that means
either
the method is more general than the particular ...TestSupport class it resides in and hence can be moved upward in the parent class.
or
the test doesn't belong to any of the genres of the existing support classes and requires a new ...TestSupport class which would extend the abstract BaseTestSupport class.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 9, 2017

Member

'module related'

Anything that is a check, fileset, etc should be extending the module one.

I think AbstractPathTestSupport must also contain getNonCompilablePath() or will this be in AbstractModuleTestSupport?

It should be in module unless you can point to use a specific non-module that must use a non-compilable file.

Member

rnveach commented Jul 9, 2017

'module related'

Anything that is a check, fileset, etc should be extending the module one.

I think AbstractPathTestSupport must also contain getNonCompilablePath() or will this be in AbstractModuleTestSupport?

It should be in module unless you can point to use a specific non-module that must use a non-compilable file.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 9, 2017

Member

Why can't we have an abstract class BaseTestSupport with constants like static final File TEST_DIR = new File(...) and abstract methods like getPath() and can also have all the other general methods in it.

This is basically what AbstractPathTestSupport is but slightly different. There are no other general methods that don't fall into one of the 2 categories we have defined.

Member

rnveach commented Jul 9, 2017

Why can't we have an abstract class BaseTestSupport with constants like static final File TEST_DIR = new File(...) and abstract methods like getPath() and can also have all the other general methods in it.

This is basically what AbstractPathTestSupport is but slightly different. There are no other general methods that don't fall into one of the 2 categories we have defined.

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 12, 2017

Issue #4592: Added AbstractPathTestSupport and CheckstyleAntTaskTest,…
… SuppressionsLoaderTest now extends from it

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 12, 2017

Issue #4592: Added AbstractPathTestSupport and CheckstyleAntTaskTest,…
… SuppressionsLoaderTest now extends from it

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 13, 2017

Issue #4592: Added AbstractPathTestSupport and CheckstyleAntTaskTest,…
… SuppressionsLoaderTest now extends from it

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 13, 2017

Issue #4592: Added AbstractPathTestSupport and CheckstyleAntTaskTest,…
… SuppressionsLoaderTest now extends from it

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 13, 2017

Issue #4592: Added AbstractPathTestSupport and CheckstyleAntTaskTest,…
… SuppressionsLoaderTest now extends from it

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

Issue #4592: Added AbstractPathTestSupport and CheckstyleAntTaskTest,…
… SuppressionsLoaderTest now extends from it
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 14, 2017

Member

@subkrish Please continue on with creating AbstractModuleTestSupport or AbstractTreeTestSupport.
Just do a small PR with new class and one existing test class changed so we can agree on contents of class like AbstractPathTestSupport.

Once these 2 are done in separate PRs, we change the rest of the tests in mass.

Member

rnveach commented Jul 14, 2017

@subkrish Please continue on with creating AbstractModuleTestSupport or AbstractTreeTestSupport.
Just do a small PR with new class and one existing test class changed so we can agree on contents of class like AbstractPathTestSupport.

Once these 2 are done in separate PRs, we change the rest of the tests in mass.

@rnveach rnveach changed the title from Class BaseCheckTestSupport should be renamed to BaseTestSupport to Split BaseCheckTestSupport into AbstractPathTestSupport, AbstractModuleTestSupport, and AbstractTreeTestSupport Jul 14, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 16, 2017

Issue #4592: Added AbstractTreeTestSupport and JavadocParseTreeTest, …
…AstTreeStringPrinterTest now extend from it

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 18, 2017

Issue #4592: Added AbstractTreeTestSupport and JavadocParseTreeTest, …
…AstTreeStringPrinterTest now extend from it

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 18, 2017

Issue #4592: Added AbstractTreeTestSupport and JavadocParseTreeTest, …
…AstTreeStringPrinterTest now extend from it

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

Issue #4592: Added AbstractTreeTestSupport and JavadocParseTreeTest, …
…AstTreeStringPrinterTest now extend from it
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 18, 2017

Member

@subkrish , are we done with this issue ?

Member

romani commented Jul 18, 2017

@subkrish , are we done with this issue ?

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 18, 2017

Contributor

@romani No, I still need to create AbstractModuleTestSupport and extend all other tests from it.

Also do I need to make changes to ITs as well? I wasn't sure about those changes. If they need to be done as well then I will work on them in this issue itself after AbstractModuleTestSupport is created.

Contributor

subkrish commented Jul 18, 2017

@romani No, I still need to create AbstractModuleTestSupport and extend all other tests from it.

Also do I need to make changes to ITs as well? I wasn't sure about those changes. If they need to be done as well then I will work on them in this issue itself after AbstractModuleTestSupport is created.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 18, 2017

Member

Also do I need to make changes to ITs as well?

yes, please, as it was created originally as some from copy UT code.

Member

romani commented Jul 18, 2017

Also do I need to make changes to ITs as well?

yes, please, as it was created originally as some from copy UT code.

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 18, 2017

Issue #4592: Added AbstractModuleTestSupport and GenericWhitespaceChe…
…ckTest, ParenPadCheckTest now extend from it

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 18, 2017

Issue #4592: Added AbstractModuleTestSupport and GenericWhitespaceChe…
…ckTest, ParenPadCheckTest, FileTabCharacterCheckTest, PackageAnnotationCheckTest now extend from it

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

Issue #4592: Added AbstractModuleTestSupport and GenericWhitespaceChe…
…ckTest, ParenPadCheckTest, FileTabCharacterCheckTest, PackageAnnotationCheckTest now extend from it
@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 23, 2017

Contributor

@romani

All test for Checks (that use verify methods) should be Test classes that extended from AbstractModuleTestSupport.
All test methods that verify ast structure should be grouped in Test classes that extends AbstractTreeTestSupport.

Exactly. But the problem is that, for example AllBlockComments uses both verify and verifyAst so it requires both the abstract classes based on the current design.

Also what about #4592 (comment) ? @rnveach suggested we make AbstractFileSetTestSupport for it and copy the method from BaseFileCheckTestSupport and have it extend AbstractPathTestSupport or AbstractModuleTestSupport based on the methods the UTs require.

Contributor

subkrish commented Jul 23, 2017

@romani

All test for Checks (that use verify methods) should be Test classes that extended from AbstractModuleTestSupport.
All test methods that verify ast structure should be grouped in Test classes that extends AbstractTreeTestSupport.

Exactly. But the problem is that, for example AllBlockComments uses both verify and verifyAst so it requires both the abstract classes based on the current design.

Also what about #4592 (comment) ? @rnveach suggested we make AbstractFileSetTestSupport for it and copy the method from BaseFileCheckTestSupport and have it extend AbstractPathTestSupport or AbstractModuleTestSupport based on the methods the UTs require.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 23, 2017

Member

But the problem is that, for example AllBlockComments uses both verify and verifyAst

As mentioned before, method https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/grammars/comments/AllBlockCommentsTest.java#L58 need to be moved out of this class as it is misplaced. Please move to AstTreeStringPrinterTest.java.

BaseFileCheckTestSupport and have it extend AbstractPathTestSupport or AbstractModuleTestSupport

Module is more abstract term than Check. I would like to avoid extra hierarchy level as much as possible. But if we come to this then BaseFileCheckTestSupport should extends AbstractModuleTestSupport. But I do not see why we need this now, please point me to a problem.

Member

romani commented Jul 23, 2017

But the problem is that, for example AllBlockComments uses both verify and verifyAst

As mentioned before, method https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/grammars/comments/AllBlockCommentsTest.java#L58 need to be moved out of this class as it is misplaced. Please move to AstTreeStringPrinterTest.java.

BaseFileCheckTestSupport and have it extend AbstractPathTestSupport or AbstractModuleTestSupport

Module is more abstract term than Check. I would like to avoid extra hierarchy level as much as possible. But if we come to this then BaseFileCheckTestSupport should extends AbstractModuleTestSupport. But I do not see why we need this now, please point me to a problem.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 23, 2017

Contributor

@romani

But I do not see why we need this now, please point me to a problem.

I don't know whether it is a problem or not but, the tests in header package and a few other tests use it. BaseFileCheckTestSupport contains one method that these test files use. As you have suggested extending it from AbstractModuleTestSupport should do the trick.

https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java#L51 will now extend AbstractModuleTestSupport instead of BaseFileTestSupport and no other changes would be made to BaseFileCheckTestSupport.

Contributor

subkrish commented Jul 23, 2017

@romani

But I do not see why we need this now, please point me to a problem.

I don't know whether it is a problem or not but, the tests in header package and a few other tests use it. BaseFileCheckTestSupport contains one method that these test files use. As you have suggested extending it from AbstractModuleTestSupport should do the trick.

https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java#L51 will now extend AbstractModuleTestSupport instead of BaseFileTestSupport and no other changes would be made to BaseFileCheckTestSupport.

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 27, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 28, 2017

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

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 28, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 29, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 29, 2017

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

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 29, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 29, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 29, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 29, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 29, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 30, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 30, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 30, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 31, 2017

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

subkrish added a commit to subkrish/checkstyle that referenced this issue Aug 1, 2017

Issue #4592: Added AbstractPathTestSupport, AbstractModuleTestSupport…
…, AbstractIndentationTestSupport and all tests in ITs extend them

subkrish added a commit to subkrish/checkstyle that referenced this issue Aug 1, 2017

Issue #4592: Added AbstractPathTestSupport, AbstractModuleTestSupport…
…, AbstractIndentationTestSupport and all tests in ITs extend them

romani added a commit that referenced this issue Aug 1, 2017

Issue #4592: Added AbstractPathTestSupport, AbstractModuleTestSupport…
…, AbstractIndentationTestSupport and all tests in ITs extend them

rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 1, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 1, 2017

romani added a commit that referenced this issue Aug 2, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 2, 2017

Member

I am closing this as all changes should be complete.
Removal of old test support class will be done in #4867 .

Member

rnveach commented Aug 2, 2017

I am closing this as all changes should be complete.
Removal of old test support class will be done in #4867 .

@rnveach rnveach closed this Aug 2, 2017

@rnveach rnveach added this to the 8.2 milestone Aug 2, 2017

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