You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2021/01/20 21:19:12 UTC
[GitHub] [incubator-gobblin] Will-Lo opened a new pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Will-Lo opened a new pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1365
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
- Travis CI is extremely slow and flaky recently, this PR aims to create a working PoC on Github Actions and disable the flaky tests for now
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561366798
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] codecov-io edited a comment on pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#issuecomment-765087860
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561363109
##########
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] asfgit closed pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] codecov-io edited a comment on pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#issuecomment-765087860
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=h1) Report
> Merging [#3206](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=desc) (a37a1ec) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/4353e0054d19e3cc898547656848511a9c700089?el=desc) (4353e00) will **decrease** coverage by `4.26%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3206 +/- ##
============================================
- Coverage 46.15% 41.89% -4.27%
+ Complexity 9764 8913 -851
============================================
Files 2017 2019 +2
Lines 77221 77506 +285
Branches 8568 8607 +39
============================================
- Hits 35645 32469 -3176
- Misses 38266 41976 +3710
+ Partials 3310 3061 -249
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/gobblin/util/filters/HiddenFilter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsdGVycy9IaWRkZW5GaWx0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| [...g/apache/gobblin/cluster/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...gobblin/runtime/mapreduce/GobblinOutputFormat.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWFwcmVkdWNlL0dvYmJsaW5PdXRwdXRGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [.../gobblin/compaction/suite/CompactionSuiteBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc3VpdGUvQ29tcGFjdGlvblN1aXRlQmFzZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...obblin/compaction/source/CompactionFailedTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25GYWlsZWRUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...n/cluster/event/ClusterManagerShutdownRequest.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvZXZlbnQvQ2x1c3Rlck1hbmFnZXJTaHV0ZG93blJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...blin/compaction/mapreduce/RecordKeyMapperBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL1JlY29yZEtleU1hcHBlckJhc2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...in/compaction/action/CompactionCompleteAction.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vYWN0aW9uL0NvbXBhY3Rpb25Db21wbGV0ZUFjdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...n/compaction/mapreduce/orc/OrcKeyDedupReducer.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL29yYy9PcmNLZXlEZWR1cFJlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...n/compaction/suite/CompactionSuiteBaseFactory.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc3VpdGUvQ29tcGFjdGlvblN1aXRlQmFzZUZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| ... and [193 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=footer). Last update [4353e00...a37a1ec](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561329401
##########
File path: gradle/scripts/ci-support.gradle
##########
@@ -0,0 +1,59 @@
+import groovy.json.JsonOutput
Review comment:
Did you miss the license file ?
##########
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:
why are we using the local address instead of dns name ?
##########
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:
Just to my understanding: we don't have to examine the gradle task status code here ? and if there's failure it just skip `break` when get into retry logic ?
##########
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:
Other than run_tests, should other job also need to specify `needs:build` ?
##########
File path: gradle/scripts/testSetup.gradle
##########
@@ -35,6 +35,7 @@ subprojects {
}
testLogging {
events "started","skipped","passed","failed"
+ exceptionFormat "full"
Review comment:
Do you know by any chance if there's a log-size limits from Github Action?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] asfgit closed pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561364158
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561450362
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] aplex commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561434195
##########
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:
If every job checks out a clean repo, then the project will get rebuilt every time. This can increase the time of CI pipeline, as things will get rebuilt 3 times. This can also introduce surprising bugs. For example, if we use a wildcard dependency, the build job can pull one version, and test job can pull a newer one.
##########
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:
What is the benefit of having multiple jobs, compared to having multiple steps (build/test/coverage) in a single job?
I see that those jobs have a lot of repeated configs, and people might update it in one place, and forget to update it in other jobs.
##########
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:
If every job checks out a clean repo, then the project will get rebuilt every time. This can increase the time of CI pipeline, as things will get rebuilt 3 times. This can also introduce surprising bugs. For example, if we use a wildcard dependency, the build job can pull one version, and test job can pull a newer one.
##########
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:
ok, we can have separate jobs for now, and later you can refactor them to use composite steps that hold common parts.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] codecov-io edited a comment on pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#issuecomment-765087860
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=h1) Report
> Merging [#3206](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=desc) (9358c3e) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/4353e0054d19e3cc898547656848511a9c700089?el=desc) (4353e00) will **decrease** coverage by `4.27%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3206 +/- ##
============================================
- Coverage 46.15% 41.88% -4.28%
+ Complexity 9764 8909 -855
============================================
Files 2017 2019 +2
Lines 77221 77506 +285
Branches 8568 8607 +39
============================================
- Hits 35645 32465 -3180
- Misses 38266 41979 +3713
+ Partials 3310 3062 -248
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/gobblin/util/filters/HiddenFilter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsdGVycy9IaWRkZW5GaWx0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| [...g/apache/gobblin/cluster/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...gobblin/runtime/mapreduce/GobblinOutputFormat.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbWFwcmVkdWNlL0dvYmJsaW5PdXRwdXRGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [.../gobblin/compaction/suite/CompactionSuiteBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc3VpdGUvQ29tcGFjdGlvblN1aXRlQmFzZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...obblin/compaction/source/CompactionFailedTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25GYWlsZWRUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...n/cluster/event/ClusterManagerShutdownRequest.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvZXZlbnQvQ2x1c3Rlck1hbmFnZXJTaHV0ZG93blJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...blin/compaction/mapreduce/RecordKeyMapperBase.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL1JlY29yZEtleU1hcHBlckJhc2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...in/compaction/action/CompactionCompleteAction.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vYWN0aW9uL0NvbXBhY3Rpb25Db21wbGV0ZUFjdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...n/compaction/mapreduce/orc/OrcKeyDedupReducer.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL29yYy9PcmNLZXlEZWR1cFJlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...n/compaction/suite/CompactionSuiteBaseFactory.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc3VpdGUvQ29tcGFjdGlvblN1aXRlQmFzZUZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| ... and [190 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=footer). Last update [4353e00...a37a1ec](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] aplex commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561434195
##########
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:
If every job checks out a clean repo, then the project will get rebuilt every time. This can increase the time of CI pipeline, as things will get rebuilt 3 times. This can also introduce surprising bugs. For example, if we use a wildcard dependency, the build job can pull one version, and test job can pull a newer one.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] codecov-io commented on pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#issuecomment-765087860
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=h1) Report
> Merging [#3206](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=desc) (962d66e) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/4353e0054d19e3cc898547656848511a9c700089?el=desc) (4353e00) will **decrease** coverage by `36.99%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3206 +/- ##
============================================
- Coverage 46.15% 9.16% -37.00%
+ Complexity 9764 1737 -8027
============================================
Files 2017 2019 +2
Lines 77221 77506 +285
Branches 8568 8607 +39
============================================
- Hits 35645 7107 -28538
- Misses 38266 69703 +31437
+ Partials 3310 696 -2614
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRmlsZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| [...n/java/org/apache/gobblin/fork/CopyableSchema.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2ZvcmsvQ29weWFibGVTY2hlbWEuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [...java/org/apache/gobblin/stream/ControlMessage.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc3RyZWFtL0NvbnRyb2xNZXNzYWdlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...va/org/apache/gobblin/dataset/DatasetResolver.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YXNldC9EYXRhc2V0UmVzb2x2ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...va/org/apache/gobblin/converter/EmptyIterable.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9FbXB0eUl0ZXJhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...org/apache/gobblin/ack/BasicAckableForTesting.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vYWNrL0Jhc2ljQWNrYWJsZUZvclRlc3RpbmcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...n/java/org/apache/gobblin/salesforce/SfConfig.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvU2ZDb25maWcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [.../org/apache/gobblin/yarn/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...va/org/apache/gobblin/cluster/SingleHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlSGVsaXhUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| [.../apache/gobblin/records/ControlMessageHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcmVjb3Jkcy9Db250cm9sTWVzc2FnZUhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| ... and [1073 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=footer). Last update [4353e00...a37a1ec](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561450362
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561452822
##########
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] aplex commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561434195
##########
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:
If every job checks out a clean repo, then the project will get rebuilt every time. This can increase the time of CI pipeline, as things will get rebuilt 3 times. This can also introduce surprising bugs. For example, if we use a wildcard dependency, the build job can pull one version, and test job can pull a newer one.
##########
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:
What is the benefit of having multiple jobs, compared to having multiple steps (build/test/coverage) in a single job?
I see that those jobs have a lot of repeated configs, and people might update it in one place, and forget to update it in other jobs.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561446318
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] aplex commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
aplex commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r562276698
##########
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:
ok, we can have separate jobs for now, and later you can refactor them to use composite steps that hold common parts.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561329401
##########
File path: gradle/scripts/ci-support.gradle
##########
@@ -0,0 +1,59 @@
+import groovy.json.JsonOutput
Review comment:
Did you miss the license file ?
##########
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:
why are we using the local address instead of dns name ?
##########
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:
Just to my understanding: we don't have to examine the gradle task status code here ? and if there's failure it just skip `break` when get into retry logic ?
##########
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:
Other than run_tests, should other job also need to specify `needs:build` ?
##########
File path: gradle/scripts/testSetup.gradle
##########
@@ -35,6 +35,7 @@ subprojects {
}
testLogging {
events "started","skipped","passed","failed"
+ exceptionFormat "full"
Review comment:
Do you know by any chance if there's a log-size limits from Github Action?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#discussion_r561363109
##########
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-gobblin] codecov-io commented on pull request #3206: [GOBBLIN-1365] Adds Github Action workflow for tests
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3206:
URL: https://github.com/apache/incubator-gobblin/pull/3206#issuecomment-765087860
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=h1) Report
> Merging [#3206](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=desc) (962d66e) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/4353e0054d19e3cc898547656848511a9c700089?el=desc) (4353e00) will **decrease** coverage by `36.99%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3206 +/- ##
============================================
- Coverage 46.15% 9.16% -37.00%
+ Complexity 9764 1737 -8027
============================================
Files 2017 2019 +2
Lines 77221 77506 +285
Branches 8568 8607 +39
============================================
- Hits 35645 7107 -28538
- Misses 38266 69703 +31437
+ Partials 3310 696 -2614
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRmlsZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| [...n/java/org/apache/gobblin/fork/CopyableSchema.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2ZvcmsvQ29weWFibGVTY2hlbWEuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [...java/org/apache/gobblin/stream/ControlMessage.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc3RyZWFtL0NvbnRyb2xNZXNzYWdlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...va/org/apache/gobblin/dataset/DatasetResolver.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YXNldC9EYXRhc2V0UmVzb2x2ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...va/org/apache/gobblin/converter/EmptyIterable.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9FbXB0eUl0ZXJhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...org/apache/gobblin/ack/BasicAckableForTesting.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vYWNrL0Jhc2ljQWNrYWJsZUZvclRlc3RpbmcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...n/java/org/apache/gobblin/salesforce/SfConfig.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvU2ZDb25maWcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [.../org/apache/gobblin/yarn/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...va/org/apache/gobblin/cluster/SingleHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlSGVsaXhUYXNrLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| [.../apache/gobblin/records/ControlMessageHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vcmVjb3Jkcy9Db250cm9sTWVzc2FnZUhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| ... and [1073 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3206/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=footer). Last update [4353e00...a37a1ec](https://codecov.io/gh/apache/incubator-gobblin/pull/3206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org