kennknowles commented on a change in pull request #12970:
URL: https://github.com/apache/beam/pull/12970#discussion_r497145208
##########
File path: sdks/java/testing/jpms-tests/build.gradle
##########
@@ -31,13 +34,66 @@ enableJavaPerformanceTesting()
description = "Apache Beam :: SDKs :: Java :: Testing :: JPMS Tests"
ext.summary = "E2E test for Java 9 modules"
+/*
+ * List of runners to run post commit tests on.
+ */
+def postCommitRunnerClass = [
+ directRunner: "org.apache.beam.runners.direct.DirectRunner",
+ flinkRunner: "org.apache.beam.runners.flink.TestFlinkRunner",
+ dataflowRunner: "org.apache.beam.runners.dataflow.TestDataflowRunner",
+]
+for (String runner : postCommitRunnerClass.keySet()) {
+ configurations.create(runner + "PostCommit")
+}
+
dependencies {
compile project(path: ":sdks:java:core", configuration: "shadow")
- compile project(path: ":runners:direct-java")
compile project(path: ":sdks:java:extensions:google-cloud-platform-core")
testCompile library.java.junit
testCompile library.java.hamcrest_core
+
+ // Add dependencies for the PostCommit configurations
+ // For each runner a project level dependency on the test project.
+ for (String runner : postCommitRunnerClass.keySet()) {
+ delegate.add(runner + "PostCommit",
project(":sdks:java:testing:jpms-tests"))
+ delegate.add(runner + "PostCommit", project(path:
":sdks:java:testing:jpms-tests", configuration: "testRuntime"))
+ }
+ directRunnerPostCommit project(":runners:direct-java")
Review comment:
Especially considering this bit, the looping over configuration doesn't
really save you much. Could be simpler to just define 3 configs with very
straightforward control flow and data flow.
##########
File path: .test-infra/jenkins/job_PostCommit_Java_Jpms_Java11.groovy
##########
@@ -39,7 +39,7 @@
PostcommitJobBuilder.postCommitJob('beam_PostCommit_Java_Jpms_Java11', 'Run Jpms
steps {
gradle {
rootBuildScriptDir(commonJobProperties.checkoutDir)
- tasks(':sdks:java:testing:jpms-tests:integrationTest')
+ tasks(':sdks:java:testing:jpms-tests:postCommit')
Review comment:
Noted below that coupling the various runners has some drawbacks. It is
some boilerplate but can we factor it out into a few jobs: direct (candidate
for precommit), flink (also candidate, but slower), dataflow (too slow for
precommit).
##########
File path: sdks/java/testing/jpms-tests/build.gradle
##########
@@ -31,13 +34,66 @@ enableJavaPerformanceTesting()
description = "Apache Beam :: SDKs :: Java :: Testing :: JPMS Tests"
ext.summary = "E2E test for Java 9 modules"
+/*
+ * List of runners to run post commit tests on.
+ */
+def postCommitRunnerClass = [
+ directRunner: "org.apache.beam.runners.direct.DirectRunner",
+ flinkRunner: "org.apache.beam.runners.flink.TestFlinkRunner",
+ dataflowRunner: "org.apache.beam.runners.dataflow.TestDataflowRunner",
+]
+for (String runner : postCommitRunnerClass.keySet()) {
Review comment:
Aesthetically, I dislike building code identifiers (or similar) by
string concat. Pragmatically it makes it hard to search for them. But looping
out these configs makes sense. What do you think about something like a map
from configuration name to runner class.
```
def postCommitConfigurations = {
"directRunnerPostCommit" : "org.apache.beam.runners.direct.DirectRunner",
"flinkRunnerPostCommit" : "org.apache.beam.runners.flink.TestFlinkRunner"
"dataflowRunnerPostCommit" :
"org.apache.beam.runners.dataflow.TestDataflowRunner"
}
```
##########
File path: sdks/java/testing/jpms-tests/build.gradle
##########
@@ -31,13 +34,66 @@ enableJavaPerformanceTesting()
description = "Apache Beam :: SDKs :: Java :: Testing :: JPMS Tests"
ext.summary = "E2E test for Java 9 modules"
+/*
+ * List of runners to run post commit tests on.
+ */
+def postCommitRunnerClass = [
+ directRunner: "org.apache.beam.runners.direct.DirectRunner",
+ flinkRunner: "org.apache.beam.runners.flink.TestFlinkRunner",
+ dataflowRunner: "org.apache.beam.runners.dataflow.TestDataflowRunner",
+]
+for (String runner : postCommitRunnerClass.keySet()) {
+ configurations.create(runner + "PostCommit")
+}
+
dependencies {
compile project(path: ":sdks:java:core", configuration: "shadow")
- compile project(path: ":runners:direct-java")
compile project(path: ":sdks:java:extensions:google-cloud-platform-core")
testCompile library.java.junit
testCompile library.java.hamcrest_core
+
+ // Add dependencies for the PostCommit configurations
+ // For each runner a project level dependency on the test project.
+ for (String runner : postCommitRunnerClass.keySet()) {
+ delegate.add(runner + "PostCommit",
project(":sdks:java:testing:jpms-tests"))
+ delegate.add(runner + "PostCommit", project(path:
":sdks:java:testing:jpms-tests", configuration: "testRuntime"))
+ }
+ directRunnerPostCommit project(":runners:direct-java")
+ flinkRunnerPostCommit project(":runners:flink:1.10")
+ dataflowRunnerPostCommit project(":runners:google-cloud-dataflow-java")
+}
+
+/*
+ * Create a ${runner}PostCommit task for each runner which runs a set
+ * of integration tests for WordCount and WindowedWordCount.
+ */
+def gcpProject = project.findProperty('gcpProject') ?: 'apache-beam-testing'
Review comment:
putting a pin in the fact that this is largely independent of your
change and applies cross module (no action required)
##########
File path: sdks/java/testing/jpms-tests/build.gradle
##########
@@ -31,13 +34,66 @@ enableJavaPerformanceTesting()
description = "Apache Beam :: SDKs :: Java :: Testing :: JPMS Tests"
ext.summary = "E2E test for Java 9 modules"
+/*
+ * List of runners to run post commit tests on.
+ */
+def postCommitRunnerClass = [
+ directRunner: "org.apache.beam.runners.direct.DirectRunner",
+ flinkRunner: "org.apache.beam.runners.flink.TestFlinkRunner",
+ dataflowRunner: "org.apache.beam.runners.dataflow.TestDataflowRunner",
+]
+for (String runner : postCommitRunnerClass.keySet()) {
+ configurations.create(runner + "PostCommit")
+}
+
dependencies {
compile project(path: ":sdks:java:core", configuration: "shadow")
- compile project(path: ":runners:direct-java")
compile project(path: ":sdks:java:extensions:google-cloud-platform-core")
testCompile library.java.junit
testCompile library.java.hamcrest_core
+
+ // Add dependencies for the PostCommit configurations
+ // For each runner a project level dependency on the test project.
+ for (String runner : postCommitRunnerClass.keySet()) {
+ delegate.add(runner + "PostCommit",
project(":sdks:java:testing:jpms-tests"))
+ delegate.add(runner + "PostCommit", project(path:
":sdks:java:testing:jpms-tests", configuration: "testRuntime"))
+ }
+ directRunnerPostCommit project(":runners:direct-java")
+ flinkRunnerPostCommit project(":runners:flink:1.10")
+ dataflowRunnerPostCommit project(":runners:google-cloud-dataflow-java")
+}
+
+/*
+ * Create a ${runner}PostCommit task for each runner which runs a set
+ * of integration tests for WordCount and WindowedWordCount.
+ */
+def gcpProject = project.findProperty('gcpProject') ?: 'apache-beam-testing'
+def gcpRegion = project.findProperty('gcpRegion') ?: 'us-central1'
+def gcsTempRoot = project.findProperty('gcsTempRoot') ?:
'gs://temp-storage-for-end-to-end-tests/'
+
+for (String runner : postCommitRunnerClass.keySet()) {
+ tasks.create(name: runner + "PostCommit", type: Test) {
+ def postCommitBeamTestPipelineOptions = [
+ "--project=${gcpProject}",
+ "--tempRoot=${gcsTempRoot}",
+ "--runner=" + postCommitRunnerClass[runner],
+ ]
+ if ("dataflowRunner".equals(runner)) {
+ postCommitBeamTestPipelineOptions.add("--region=${gcpRegion}")
+ }
+ classpath = configurations."${runner}PostCommit"
+ include "**/*IT.class"
+ maxParallelForks 4
+ systemProperty "beamTestPipelineOptions",
JsonOutput.toJson(postCommitBeamTestPipelineOptions)
+ }
+}
+
+/* Define a common postcommit task which depends on all the individual
postcommits. */
Review comment:
This is a laudable instinct, but I actually don't favor it. Reasons:
1. Prefer to separate test suites from when they may run
2. Runners often have fairly different infrastructures to spin up and
potentially blow memory if accidentally run together
3. We want separate signals about what is unhealthy.
Regarding naming of a test suite "postcommit" I want to acknowledge that you
are noticing how things are currently organized and following those
conventions. So that's sensible. But it is a convention I think we have to move
away from. Gradle can be run from lots of places and the notion of precommit vs
postcommit is inessential.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
