Testing conventions task part 2 #36107
Conversation
Covers situtatuins as descrobed in elastic#35435 where test classes don't have tasks that include them.
u You are currently bisecting, started from branch 'master'.
So the check doesn't complain that the tests are not ran
alpar-t
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
I like it! I see why having this in the gradle process is nice too! I left a few comments. I'm unsure about removing that SEQUENCE check, mostly because I don't know what it does.
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java
Outdated
Show resolved
Hide resolved
| systemProperty 'tests.gradle_unreleased_versions', bwcVersions.unreleased.join(',') | ||
| } | ||
|
|
||
| task integTest(type: RandomizedTestingTask) { |
There was a problem hiding this comment.
I'd probably rename the test to match the normal suffix.
There was a problem hiding this comment.
Not sure what you mean.
There was a problem hiding this comment.
I mean, instead of adding the new test run, I'd fix the test's name to match the normal "Tests" suffix.
There was a problem hiding this comment.
There's just a few of these tests, but they all extend ESIntegTestCase . I think that calling them Tests rather than IT would go against the naming convention.
There was a problem hiding this comment.
It depends on where they are in the project. If they were in the server, yes. Otherwise, we do tend to call the Tests. The naming convention isn't super consistent, sadly.
| public void testReproducible() throws IOException { | ||
| if (ITER++ == 0) { | ||
| CLUSTER_SEED = cluster().seed(); | ||
| for (int i = 0; i < SEQUENCE.length; i++) { |
There was a problem hiding this comment.
I'm not sure if this is ok to remove.
There was a problem hiding this comment.
I honestly haven't thought enough about it to be sure.
There was a problem hiding this comment.
Maybe I should have looked more into it before removing it.
For one, it doesn't passes. I think that is because a new sub-seed is generated for each iteration from a master seed, so it seems the test was not written correctly.
On the other hand, it seems to test the randomized runner only.
Not saying it's useless but probably not the greatest value, did not seem wroth it to write at this time.
There was a problem hiding this comment.
I'd hunt down the original author before dropping it. I think the best thing to do at this point is mark it as @AwaitsFix.
There was a problem hiding this comment.
Thanks for sticking out with this @nik9000 you were right, I was to quick to change the test. I @AwaitFixed it instead.
|
@nik9000 ready for another review |
|
@elasticmachine test this please |
|
@atorok, I left a few small things. Almost! Thanks for pinging me on it! |
| systemProperty 'tests.gradle_unreleased_versions', bwcVersions.unreleased.join(',') | ||
| } | ||
|
|
||
| task integTest(type: RandomizedTestingTask) { |
There was a problem hiding this comment.
It depends on where they are in the project. If they were in the server, yes. Otherwise, we do tend to call the Tests. The naming convention isn't super consistent, sadly.
Closes #35435 - make it easier to add additional testing tasks with the proper configuration and add some where they were missing. - mute or fix failing tests - add a check as part of testing conventions to find classes not included in any testing task.
5 participants
Closes #35435
Changes to verify that we don't have test classes which are not ran as part of
./gradlew check