-
Notifications
You must be signed in to change notification settings - Fork 26
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
[#382, #241] Add isTestableClass and isTestableMethod to determine testable classes and methods #461
base: development
Are you sure you want to change the base?
[#382, #241] Add isTestableClass and isTestableMethod to determine testable classes and methods #461
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good, works as expected. However, we probably should discuss my concern annotated in JavaPsiClassWrapper
, that there could be more cases of non-testable classes.
Besides, having tests for those cases would be an extra gimmick 😉
} | ||
|
||
// Check if the class has methods annotated with @Test | ||
val areAllMethodsTestable = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name does not reflect what the variable actually holds, namely if there is at least one method annotated with @Test
. I suggest hasTestableMethod
.
|
||
// Check if the class explicitly subclasses a known test framework class (e.g., TestCase) | ||
if (InheritanceUtil.isInheritor(psiClass, "junit.framework.TestCase")) return false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see an additional case here: it could be the case that the CUT does not have @Test
methods but provides fixture methods, e.g., annotated with @BeforeEach
and is only used as a base class for the actual test classes. I have done this before to group shared logic of multiple tests and I have also seen that elsewhere. In that case, the CUT neither is a testable class, IMHO.
An example could be (minimised to basically nonsense but it show the structure hopefully):
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
class Base {
protected Object foo;
@BeforeEach
void setUp() {
foo = new Object();
}
}
class FirstTest extends Base {
@Test
void test() {
assert foo != null;
}
}
class SecondTest extends Base {
@Test
void test() {
assert foo.equals(foo);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see, there is not reliable way to detect whether a file is part of the testing tree or not. Even searching the file path for something like test
is just an heuristics, think of it
for integration tests, for example.
The most reliable heuristics I can think of in the moment would be that we check for specific imports. A file importing something from org.junit.jupiter.api
would be a candidate for something test-related, because that is the JUnit 5 APIs for writing tests. JUnit 4 makes it much harder because everything lives under org.junit
, however, for example for implementations that use the runner APIs, we probably want to be able to generate tests. Other candidates could be com.google.guava
or org.hamcrest
but this list will probably never be complete.
What do you think?
if (superClassFqName == "junit.framework.TestCase") { | ||
return false | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my annotation in JavaPsiClassWrapper
I assume that similar stuff is possible in Kotlin, too, so there might be more cases here.
Description of changes made
Imporve checkings of the classes to test and methods to test requirments/
Other notes
Closes #382
Closes #241