Introducing testing conventions task #35861
Conversation
alpar-t
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
|
@elasticmachine run gradle tests 1 |
|
@elasticmachine run gradle build tests 1 |
1 similar comment
|
@elasticmachine run gradle build tests 1 |
alpar-t
force-pushed
the
testing-conventions-task
branch
from
5e64218 to
3a80432
Compare
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
I like it! I left a minor thing. We can build on this and remove the other checks that we have as we go! wonderful.
| private static final String TEST_CLASS_SUFIX = "Tests"; | ||
| private static final String INTEG_TEST_CLASS_SUFIX = "IT"; | ||
| private static final String TEST_METHOD_PREFIX = "test"; | ||
| private final List<String> problems; |
There was a problem hiding this comment.
It feels like this should be a method variable in the test() method.
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
One question. I'm also curious how long this takes vs the previous task testing conventions implementation?
| public abstract class Boilerplate { | ||
|
|
||
| public static SourceSetContainer getJavaSourceSets(Project project) { | ||
| return project.getConvention().getPlugin(JavaPluginConvention.class).getSourceSets(); |
There was a problem hiding this comment.
Why does this need to be in a separate class?
There was a problem hiding this comment.
I just updated the FilePermissionsTask to use it too. I suspect there are others too, and there are possibly other cases where something that's straight forward in Groovy is lengthy in java. I wanted to create a home for these.
There was a problem hiding this comment.
The name is a bit funny to me but I understand why you'd want something for this.
|
@rjernst |
|
@elasticmachine run gradle build tests 1 |
Is that including compile or just the running the task? Does it include gradle's startup time? Or is it the runtime of running that task for all subprojects? |
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
I'm happy with it modulo the timing discussion.
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
LGTM. I also think after we convert the existing namingConventions uses, we should change this task name back to namingConventions
|
The times are what Gradle reports after running each task after a clean has previously been ran, using the daemon and parallel. So it does include compile times as well. |
6 participants
This is in the context of #33524 and #34820 and an annoyance I had with running these checks as a separate process.
This PR introduces a new task meant to replace
NamingConventionsCheckandNamingConventionsTask.In the interest of keeping the PR small it does not do so yet, but the goal is to have all the checks implemented here.
These checks will be added as follow ups and we will eventually replace the old tasks with the new porting the existing tests to the new task.
There are small changes in the way the checks are done, like looking for method naming connections and methods annotated with
@Test.The advantage of doing this in Gradle is speed and the ability to gather insights.
This will allow us to do things like auto disable testing tasks if all tests are muted.
We call the same reflection methods multiple times, like getting the methods of a class multiple times to keep what's being checked for more readable.