Will-Lo commented on a change in pull request #3206: URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561360556
########## File path: .github/workflows/build_and_test.yaml ########## @@ -0,0 +1,162 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Build and Run Tests + +on: + push: + # Publish only on `master` + branches: + - master + pull_request: + branches: + - master + release: + types: [published, edited] + +jobs: + build: + name: Build repository + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Build repository + run: | + ./gradlew --no-daemon clean build -x test -x javadoc -x findbugsMain -x findbugsTest -x checkstyleMain -x checkstyleJmh -x checkstyleTest -Dorg.gradle.parallel=true + + test_coverage: + runs-on: ubuntu-latest + name: Generate test coverage + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Generate code coverage + run: | + ./gradlew -PskipTestGroup=disabledOnCI -Dorg.gradle.parallel=false -DjacocoBuild=1 $GOBBLIN_GRADLE_OPTS jacocoTestCoverage + + static_checks: + name: Run static checks + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Run CheckStyle and FindBugs + run: | + ./gradlew --no-daemon -x javadoc findbugsMain checkstyleMain checkstyleTest checkstyleJmh + + run_tests: + timeout-minutes: 60 + env: + GOBBLIN_GRADLE_OPTS: "--no-daemon -Dgobblin.metastore.testing.embeddedMysqlEnabled=false -PusePreinstalledMysql=true" + strategy: + matrix: + test-group: ["Core Tests", "Service Tests", "Module Tests", "Other Tests"] + fail-fast: false + runs-on: ubuntu-latest + needs: build + services: + mysql: + image: mysql:5.7.32 + env: + MYSQL_USER: testUser + MYSQL_PASSWORD: testPassword + MYSQL_DATABASE: test + MYSQL_ROOT_PASSWORD: password + ports: + - 3306:3306 + options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=5 + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + - name: Verify mysql connection + run: | + sudo apt-get install -y mysql-client + mysql --host 127.0.0.1 --port 3306 -uroot -ppassword -e "SHOW DATABASES" + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Run test group ${{ matrix.test-group }} + # Write retry logic as integration tests can fail due to timing out or network problems + run: | + ./gradlew getGroupedTests -PgroupName="${{matrix.test-group}}" > temp.txt + TASKS=$(sed -n 's/CI Task: //p' temp.txt) + echo $TASKS + + n=0 + until [ "$n" -ge 3 ] + do + ./gradlew -PskipTestGroup=disabledOnCI $GOBBLIN_GRADLE_OPTS $TASKS -Dorg.gradle.parallel=false && break Review comment: Yup, the gradle task automatically returns exit code 1 here and aborts. So if it passes it'll call `break`, otherwise it'll be evaluated to false and keep looping until it attempts 3 times. ########## File path: .github/workflows/build_and_test.yaml ########## @@ -0,0 +1,162 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Build and Run Tests + +on: + push: + # Publish only on `master` + branches: + - master + pull_request: + branches: + - master + release: + types: [published, edited] + +jobs: + build: + name: Build repository + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Build repository + run: | + ./gradlew --no-daemon clean build -x test -x javadoc -x findbugsMain -x findbugsTest -x checkstyleMain -x checkstyleJmh -x checkstyleTest -Dorg.gradle.parallel=true + + test_coverage: Review comment: So interestingly enough none of the builds need to be dependent on each other, because gradle should be building the files needed if it hasn't been done so yet on the runner. But I can make the others dependent on `build` too, since I think it would make more logical sense ########## File path: .github/workflows/build_and_test.yaml ########## @@ -0,0 +1,162 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Build and Run Tests + +on: + push: + # Publish only on `master` + branches: + - master + pull_request: + branches: + - master + release: + types: [published, edited] + +jobs: + build: + name: Build repository + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Build repository + run: | + ./gradlew --no-daemon clean build -x test -x javadoc -x findbugsMain -x findbugsTest -x checkstyleMain -x checkstyleJmh -x checkstyleTest -Dorg.gradle.parallel=true + + test_coverage: Review comment: So interestingly enough none of the jobs need to be dependent on each other because gradle should be building the files needed if it hasn't been done so yet on the runner. But I can make the others dependent on `build` too, since I think it would make more logical sense ########## File path: gobblin-metastore/src/test/java/org/apache/gobblin/metastore/testing/TestMetastoreDatabaseServer.java ########## @@ -110,7 +111,7 @@ static Config getDefaultConfig() { .put(EMBEDDED_MYSQL_ENABLED_FULL_KEY, true) .put(DBUSER_NAME_FULL_KEY, "testUser") .put(DBUSER_PASSWORD_FULL_KEY, "testPassword") - .put(DBHOST_FULL_KEY, "localhost") + .put(DBHOST_FULL_KEY, "127.0.0.1") Review comment: Ah this was left in when I was trying to debug mysql connections I'll change it back to localhost. ########## File path: gradle/scripts/testSetup.gradle ########## @@ -35,6 +35,7 @@ subprojects { } testLogging { events "started","skipped","passed","failed" + exceptionFormat "full" Review comment: There isn't as far as I know, i needed to add this to gain some insight on failed tests since I don't think there's a way to access the gradle test html after tests. ########## File path: .github/workflows/build_and_test.yaml ########## @@ -0,0 +1,164 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Build and Run Tests + +on: + push: + # Publish only on `master` + branches: + - master + pull_request: + branches: + - master + release: + types: [published, edited] + +jobs: + build: + name: Build repository + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Build repository + run: | + ./gradlew --no-daemon clean build -x test -x javadoc -x findbugsMain -x findbugsTest -x checkstyleMain -x checkstyleJmh -x checkstyleTest -Dorg.gradle.parallel=true + + test_coverage: + runs-on: ubuntu-latest + name: Generate test coverage + needs: build + steps: Review comment: Jobs have the ability to be run in parallel, and steps do not. Since the test coverage and static checks are independent of the unit tests, it would be better to separate them from the normal build process to speed up the pipeline. I agree that the configs are repeated a lot. It is possible to write composite steps in Github actions so that we can reuse them in multiple jobs though. https://docs.github.com/en/actions/creating-actions/creating-a-composite-run-steps-action. I can implement it in this PR or put it in a different PR, depending on how high you want to prioritize it. ########## File path: .github/workflows/build_and_test.yaml ########## @@ -0,0 +1,162 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Build and Run Tests + +on: + push: + # Publish only on `master` + branches: + - master + pull_request: + branches: + - master + release: + types: [published, edited] + +jobs: + build: + name: Build repository + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Build repository + run: | + ./gradlew --no-daemon clean build -x test -x javadoc -x findbugsMain -x findbugsTest -x checkstyleMain -x checkstyleJmh -x checkstyleTest -Dorg.gradle.parallel=true + + test_coverage: Review comment: Yup that's a good point, I made the other jobs dependent on the first build job so now it should only download dependencies once. The dependencies should be stored by the gradle cache after `build` which is shared across jobs, but if it's wildcard then gradle might force a dependency download/check. I think in this scenario if it causes a problem it's acceptable as the later steps would have the newer dependency, and if it causes the tests to break then we should really be building against an exact version. ########## File path: .github/workflows/build_and_test.yaml ########## @@ -0,0 +1,162 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Build and Run Tests + +on: + push: + # Publish only on `master` + branches: + - master + pull_request: + branches: + - master + release: + types: [published, edited] + +jobs: + build: + name: Build repository + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Build repository + run: | + ./gradlew --no-daemon clean build -x test -x javadoc -x findbugsMain -x findbugsTest -x checkstyleMain -x checkstyleJmh -x checkstyleTest -Dorg.gradle.parallel=true + + test_coverage: Review comment: Yup that's a good point, I made the other jobs dependent on the first build job so now it should only download dependencies once. The dependencies should be stored by the gradle cache after `build` which is shared across jobs, but if it's wildcard then gradle might force a dependency download/check. I think in this scenario if it causes a problem it's acceptable as the later steps would have the newer dependency, and if it causes the tests to break then we should really be building against an exact version. ########## File path: .github/workflows/build_and_test.yaml ########## @@ -0,0 +1,164 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Build and Run Tests + +on: + push: + # Publish only on `master` + branches: + - master + pull_request: + branches: + - master + release: + types: [published, edited] + +jobs: + build: + name: Build repository + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v2 + - name: Set up JDK 1.8 + uses: actions/setup-java@v1 + with: + java-version: 1.8 + # Stores external dependencies, can be further improved with Gradle 6.1 + - name: Cache Gradle Dependencies + uses: actions/cache@v2 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + # Only rebuild cache if build.gradle is changed + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} + restore-keys: ${{ runner.os }}-gradle + - name: Build repository + run: | + ./gradlew --no-daemon clean build -x test -x javadoc -x findbugsMain -x findbugsTest -x checkstyleMain -x checkstyleJmh -x checkstyleTest -Dorg.gradle.parallel=true + + test_coverage: + runs-on: ubuntu-latest + name: Generate test coverage + needs: build + steps: Review comment: Responding to your first comment, Yup that's a good point, I made the other jobs dependent on the first build job so now it should only download dependencies once. The dependencies should be stored by the gradle cache after `build` which is shared across jobs, but if it's wildcard then gradle might force a dependency download/check. I think in this scenario if it causes a problem it's acceptable as the later steps would have the newer dependency, and if it causes the tests to break then we should really be building against an exact version. ---------------------------------------------------------------- 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]
