Testing conventions: add support for checking base classes#36650
Testing conventions: add support for checking base classes#36650alpar-t merged 17 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-core-infra |
|
|
||
| @SuppressWarnings("unused") | ||
| public void testUsage() throws IOException, InterruptedException { | ||
| public void usage() throws IOException, InterruptedException { |
There was a problem hiding this comment.
These are for documentation only, not tests, it's not code we run and the method name is not important.
I changed it because testing conventions picked up the method name.
There was a problem hiding this comment.
I thought we did execute these. In my opinion, it is important to execute the examples that we have in the docs. They may not assert all that much, but at a minimum they shouldn't crash and it'd be nice if we made them assert things that matched the prose. Which we often do and might do here as well.
rjernst
left a comment
There was a problem hiding this comment.
I see the usefulness for this between the gradle testing side and the rest of our code. But, outside of server's integTest (which we can fix by renaming to internalClusterTest as xpack already has), where do we have divergence from the set naming conventions? While I understand the advantage of general purpose configurability for gradle plugins in general, making these configurable within our own project lends to different subprojects having different conventions. I think consistency across so many subprojects is paramount. So, I am ok with this PR from the perspective of making it configurable for build-tools to use, but I think it should be configured programmatically so that when we construct the naming convention task, it is set and cannot be changed.
| } | ||
|
|
||
| testingConventions { | ||
| naming.clear() |
There was a problem hiding this comment.
Why do we need this clear()? I don't like the leniency it provides. If we want to provide the ability to replace completely, then let's have a set version (much like many task settings can be set with = instead of calling DSL-like methods for adding).
There was a problem hiding this comment.
We do have a set method, so one can write something like baseClasses = [...] to replace what was already there. The clear comes from the Gradle container in which the rules live and it's consistent with how Gradle behaves in general. Granted the clear method is not used often. The intent here is to remove a rule from the container. The default is rule for Tests and IT. If a project doesn't have both, the alternative is naming.remove(naming.getByName('Tests')). The semantics of clear are I think slightly better, because it expresses "I only have IT", rather than "I don't have Tests", it's also indicative that all the rules are configured right here and there can be nothing coming from configuration elsewhere. I don't hover have a strong preference.
I don't like it in general where we have a plugin configure something and then the project throws it out completely. We sometimes do this with disabling tasks as well.
There should be a way to ask only for what the project needs.
I also explain in the comment bellow that I view this as an intermediary step and don't want to keep rules as configurable, it's just a means to get to the point where everything follows conventions.
| import java.util.Set; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| public class TestingConventionRule implements Serializable { |
There was a problem hiding this comment.
Please add at least minimal javadocs. What would you want someone who has never seen this class to understand about it if they navigate here while reading through build code?
|
@rjernst I fully agree with what you said. Making it configurable defeats the purpose of conventions. I see this as the path to being able to enforce the conventions in full. |
…conventions-base-classes
There was a problem hiding this comment.
Thanks @atorok for the explanation. I'm ok moving forward with this, but want to be sure this state does not exist for long. Historically, anytime something like this is configurable, someone will (ab)use it somehow, and it makes it more difficult to remove.
| */ | ||
| public class TestingConventionRule implements Serializable { | ||
|
|
||
| private final String name; |
There was a problem hiding this comment.
I think this should be "suffix" instead of "name"?
Closes #33524
The PR is not as big as it seems, it adds a bunch of dummy classes to cover testing scenarios.
With this PR testing conventions can check that tests with a specific suffix extend from a specific list of base classes.
Both the suffixes and the base classes are configurable.
This now covers all the checks of
namingConventionswhich will be removed in a subsequent PR.We now also have tests for all the checks that naming conventions implement,
for good reason because the test uncovered that problems were not reported as they should. That's also fixed now.
Functionality to look for tasks in subprojects that run the tests is removed, and we disable the check on the parents instead. This was not working as intended and it's tricky to implement correctly because the tests are compiled for each subproject so we would need to load the test classes of the sub-projects too.
It's not worth the effort since we eventually check those tests for all sub-projects and would
catch any miss-configurations.
There are some other fixes to make the checks pass, like enabling integ tests on
sqlto pick up some tests that don't currently.