Skip to content
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.

Sign up for GitHub

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

Replace usages RandomizedTestingTask with built-in Gradle Test #40564

Merged
merged 44 commits into from
Apr 5, 2019
Merged

Replace usages RandomizedTestingTask with built-in Gradle Test #40564

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
2d9c898
Replace usages RandomizedTestingTask with built-in Gradle Test
mark-vieira Mar 27, 2019
0e9a24c
Workaround for test conventions enforcement
mark-vieira Mar 28, 2019
6536c59
Merge remote-tracking branch 'origin/master' into gradle-test-runner
mark-vieira Mar 28, 2019
8bb56d4
Fix up some merge conflicts
mark-vieira Mar 28, 2019
22d4db8
Use existing mechanism for determining test parallelism
mark-vieira Mar 28, 2019
5437841
Fix overzelous find and replace
mark-vieira Mar 28, 2019
a6e23f2
Tweak test event output
mark-vieira Mar 28, 2019
fb8e639
Log strait to std streams as it's faster and doesn't introduce extra …
mark-vieira Mar 28, 2019
33a5cd3
Remove unnecessary conditional
mark-vieira Mar 28, 2019
8081e84
Remove unnecessary check
mark-vieira Mar 28, 2019
ee88d66
Prefix test log output with std stream identifier
mark-vieira Mar 28, 2019
9e882d8
Fix overzelous find and replace
mark-vieira Mar 28, 2019
7df907d
Fix overzelous find and replace
mark-vieira Mar 28, 2019
d63296a
Fix overzelous find and replace
mark-vieira Mar 28, 2019
ceecad6
Fix overzelous find and replace
mark-vieira Mar 28, 2019
371dba7
Simplify parallelism logic
mark-vieira Mar 28, 2019
71a7bb2
Disable test tasks when Docker is required but not available
mark-vieira Mar 28, 2019
313fb10
Revert "Remove unnecessary conditional"
mark-vieira Mar 28, 2019
d968dd2
Fix issue with tests hanging on Java 8 and lower
mark-vieira Mar 29, 2019
4ad3b25
Convert ErrorReportingTestListener to Java
mark-vieira Mar 29, 2019
38903e7
Ignore problematic system properties
mark-vieira Mar 29, 2019
3590f53
Cap parallelism if we can't determine physical cores available
mark-vieira Mar 29, 2019
3364048
Cap parallelism if we can't determine physical cores available
mark-vieira Mar 29, 2019
e773c64
Catch situations where we don't properly apply unit test config
mark-vieira Mar 29, 2019
0ee1a0c
Another lazy gstring system property
mark-vieira Mar 29, 2019
e7a504f
Merge remote-tracking branch 'origin/master' into gradle-test-runner
mark-vieira Mar 29, 2019
dfc82b9
Simply parallel forks logic
mark-vieira Mar 29, 2019
6b6b5c4
Use gradle `--tests` option in test failure reproduction line
mark-vieira Mar 29, 2019
abf29c3
More lazy gstring system properties
mark-vieira Mar 29, 2019
1d74b97
Ditch usages of DefaultGroovyMethods.
mark-vieira Mar 29, 2019
636c067
Ditch extra newlines
mark-vieira Mar 29, 2019
3d19d70
Fix lazy task config
mark-vieira Mar 29, 2019
148a964
Fix integ test system properties when using testclusters plugin
mark-vieira Mar 29, 2019
12f454e
Fix attachment processor tests
mark-vieira Mar 30, 2019
c2951ce
Merge remote-tracking branch 'origin/master' into gradle-test-runner
mark-vieira Mar 30, 2019
7801b8f
Merge remote-tracking branch 'origin/master' into gradle-test-runner
mark-vieira Apr 1, 2019
4fe39f5
Merge remote-tracking branch 'origin/master' into gradle-test-runner
mark-vieira Apr 1, 2019
a245b22
Don't both setting the test seed on all subprojects
mark-vieira Apr 2, 2019
cf9b01b
Improve test failure reporting
mark-vieira Apr 2, 2019
7cc13b2
Merge remote-tracking branch 'origin/master' into gradle-test-runner
mark-vieira Apr 3, 2019
5e0b83c
Merge remote-tracking branch 'origin/master' into gradle-test-runner
mark-vieira Apr 4, 2019
429a506
Remove duplicate code in overridden method
mark-vieira Apr 4, 2019
395a8ed
Mark logger final
mark-vieira Apr 4, 2019
8d67e0d
Declare variables with explicit types
mark-vieira Apr 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmarks/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ mainClassName = 'org.openjdk.jmh.Main'
assemble.enabled = false
archivesBaseName = 'elasticsearch-benchmarks'

unitTest.enabled = false
test.enabled = false

dependencies {
compile("org.elasticsearch:elasticsearch:${version}") {
Expand Down
9 changes: 3 additions & 6 deletions buildSrc/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,24 +199,21 @@ if (project != rootProject) {
into localDownloads
}

unitTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was a bit forced on us since we couldn't replace test, but I don't think we should revert it as it brings clarity.

Copy link
Contributor Author

@mark-vieira mark-vieira Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure all projects having a test task that does nothing, in addition to a unitTest task is particularly clear. I'm not sure we should continue with this funny convention just for historical purposes. We can now also ditch all the goofy logic to disable it, as well as inherit the normal conventions for this task rather than duplicating what the java plugin already does for us. Seems unnecessary to me and as you say, this naming was only introduced in the first place due to a technical limitation.

I don't think educating folks that :test == unit tests is asking a lot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change wasn't done only as a technical limitations.
I do and always have hated the Gradle test task for the ambiguity it brings,
and for running IT by default, it makes it too easy to mangle tests of different kinds, and it really probably only exists for historical reasons because other build tools had it.
Also, the fact that it's being changed in this PR has nothing to do with the goal of the PR and just makes it much harder to read.

My goal is to have specific unitTest, integTest, and internalClusterTest tasks with well defined meanings eventually. test doesn't contribute to this clarity,
it's just a catch all which makes it seem ok to add stuff there and move on, whereas unitTest makes one think about what tests are being added and what's the best place for them.

Gradle is broken in that it enforces a testing conventions that does not scale for anything but projects of trivial size with no convenient way to opt-out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do and always have hated the Gradle test task for the ambiguity it brings,
and for running IT by default, it makes it too easy to mangle tests of different kinds,

To be fair, I think you are just describing Maven surefire and failsafe plugin behaviors. Which themselves are a workaround to a technical limitation which is that Maven doesn't support more than two source sets per module. The proper way to separate tests like this is to model them as separate source sets, not just different Test tasks with includes/excludes based on naming conventions. This has lots of other benefits as well.

My goal is to have specific unitTest, integTest, and internalClusterTest tasks with well defined meanings eventually.

I think we are bikeshedding to some degree. Again, I don't think it's a big deal to convey that test runs unit tests, and other tests run the tests as the name describes.

Gradle is broken in that it enforces a testing conventions that does not scale for anything but projects of trivial size with no convenient way to opt-out.

I think it's the opposite actually. It doesn't enforce any conventions, which I think is your complaint. I think the "convention" you have issue with is the name of the task it creates. This is really the only thing Gradle is "forcing" on us.

test {
// The test task is configured to runtimeJava version, but build-tools doesn't support all of them, so test
// with compiler instead on the ones that are too old.
if (project.runtimeJavaVersion <= JavaVersion.VERSION_1_10) {
jvm = "${project.compilerJavaHome}/bin/java"
executable = "${project.compilerJavaHome}/bin/java"
}
}

// This can't be an RandomizedTestingTask because we can't yet reference it

task integTest(type: Test) {
// integration test requires the local testing repo for example plugin builds
dependsOn project.rootProject.allprojects.collect {
it.tasks.matching { it.name == 'publishNebulaPublicationToLocalTestRepository'}
}
dependsOn setupLocalDownloads
exclude "**/*Tests.class"
testClassesDirs = sourceSets.test.output.classesDirs
classpath = sourceSets.test.runtimeClasspath
inputs.dir(file("src/testKit"))
// tell BuildExamplePluginsIT where to find the example plugins
systemProperty (
Expand Down
53 changes: 0 additions & 53 deletions buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/BalancersConfiguration.groovy
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/ListenersConfiguration.groovy
View file
Open in desktop

This file was deleted.

60 changes: 0 additions & 60 deletions buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/RandomizedTestingPlugin.groovy
View file
Open in desktop

This file was deleted.

Loading