You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mumrah (via GitHub)" <gi...@apache.org> on 2024/09/05 22:35:14 UTC
[PR] MINOR Fix ignoreFailures logic in CI workflow [kafka]
mumrah opened a new pull request, #17106:
URL: https://github.com/apache/kafka/pull/17106
This property was removed in #17066 to prevent test failures from being cached. However, this breaks the JUnit report and makes the github workflow less user friendly.
The problem is that we are copying the junit test report files into a new directory (added in #17098) in a Gradle `doLast` closure. If we don't run with `ignoreFailures=true`, then this closure will not run and the test failures won't be processed by junit.py.
This patch adds logic to our `:test` tasks to always run with `ignoreFailures=true`, but still fail the task if any tests fail. The user provided `-PignoreFailures` is still used for unitTest and integrationTest tasks.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah merged PR #17106:
URL: https://github.com/apache/kafka/pull/17106
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1746392502
##########
build.gradle:
##########
@@ -499,6 +502,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
+ ext.hadFailure = true
+ }
+ }
+
+ // This closure will copy JUnit XML files out of the sub-project's build directory and into
+ // a top-level build/junit-xml directory. This is necessary to avoid reporting on tests which
+ // were not run, but instead were restored via FROM-CACHE. See KAFKA-17479 for more details.
+ //
+ // This closure will not run if a test fails and ignoreFailures is not true.
+ doLast {
+ def dest = rootProject.layout.buildDirectory.dir("junit-xml/${project.name}").get().asFile
+ println "Copy JUnit XML for ${project.name} to $dest"
+ ant.copy(todir: "$dest") {
+ ant.fileset(dir: "${test.reports.junitXml.entryPoint}")
+ }
+
+ // If there were any test failures, we want to fail the task to prevent the failures
+ // from being cached.
+ if (ext.hadFailure) {
Review Comment:
😄 I was thinking the same. I just pushed an update to preserve the existing `ignoreFailures` behavior
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1747282633
##########
build.gradle:
##########
@@ -499,6 +502,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
Review Comment:
I did try `finalizedBy`, unfortunately it is always run even if a task was UP-TO-DATE or FROM-CACHE. This results in all the XML files being copied, even if they were from the cache.
Back here https://github.com/apache/kafka/pull/17066#discussion_r1744071589 you suggested using the GITHUB_ACTIONS env as a way to control caching. I think we can do something similar here.
If GITHUB_ACTIONS is set, we'll always set `ignoreFailures=true`. This ensures our `doLast` is called and the files are copied. Since the CI pipeline relies on the downstream "Parse JUnit Tests" job for detecting failures, we don't care if Gradle exits with 1 or 0.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1747387877
##########
build.gradle:
##########
@@ -499,6 +502,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
Review Comment:
> If GITHUB_ACTIONS is set, we'll always set ignoreFailures=true. This ensures our doLast is called and the files are copied. Since the CI pipeline relies on the downstream "Parse JUnit Tests" job for detecting failures, we don't care if Gradle exits with 1 or 0.
that is a better approach :)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #17106:
URL: https://github.com/apache/kafka/pull/17106#issuecomment-2334284335
Locally, I added a test that will always be flaky (fail, then pass) in the `:metadata` module. Here are the results with the latest code 111fcf7a895.
Before each run, I emptied the build cache. Here is the base command I used:
```
./gradlew --build-cache --info --no-scan --continue :metadata:clean :metadata:test
```
Results
| env | maxTestRetries | ignoreFailures | gradle exit | xml files moved | cache saved | comment |
| --- | -------------- | --------------- | ----------- | ---------------- | ------------- | --------- |
| - | 0 | false | 1 | no | no | default invocation |
| - | 1 | false | 0 | no | yes |
| - | 0 | true | 0 | no | yes |
| - | 1 | true | 0 | no | yes |
| GITHUB_ACTIONS | 0 | false (i) | 1 | yes | no |
| GITHUB_ACTIONS | 1 | false (i) | 1 | yes | no | suggested CI invocation (ii) |
i) ignoreFailures is overwritten by GITHUB_ACTIONS
ii) relies on "Parse JUnit tests" to fail the overall pipeline
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1746384913
##########
build.gradle:
##########
@@ -499,6 +502,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
+ ext.hadFailure = true
+ }
+ }
+
+ // This closure will copy JUnit XML files out of the sub-project's build directory and into
+ // a top-level build/junit-xml directory. This is necessary to avoid reporting on tests which
+ // were not run, but instead were restored via FROM-CACHE. See KAFKA-17479 for more details.
+ //
+ // This closure will not run if a test fails and ignoreFailures is not true.
+ doLast {
+ def dest = rootProject.layout.buildDirectory.dir("junit-xml/${project.name}").get().asFile
+ println "Copy JUnit XML for ${project.name} to $dest"
+ ant.copy(todir: "$dest") {
+ ant.fileset(dir: "${test.reports.junitXml.entryPoint}")
+ }
+
+ // If there were any test failures, we want to fail the task to prevent the failures
+ // from being cached.
+ if (ext.hadFailure) {
Review Comment:
Should we honor the "ignoreFailures=true" in this condition?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #17106:
URL: https://github.com/apache/kafka/pull/17106#issuecomment-2334631676
@chia7712 I realized I still did not address the timeout case. I'll fix that in a follow-up shortly.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1746392320
##########
build.gradle:
##########
@@ -499,6 +502,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
+ ext.hadFailure = true
+ }
+ }
+
+ // This closure will copy JUnit XML files out of the sub-project's build directory and into
+ // a top-level build/junit-xml directory. This is necessary to avoid reporting on tests which
+ // were not run, but instead were restored via FROM-CACHE. See KAFKA-17479 for more details.
+ //
+ // This closure will not run if a test fails and ignoreFailures is not true.
+ doLast {
+ def dest = rootProject.layout.buildDirectory.dir("junit-xml/${project.name}").get().asFile
+ println "Copy JUnit XML for ${project.name} to $dest"
+ ant.copy(todir: "$dest") {
+ ant.fileset(dir: "${test.reports.junitXml.entryPoint}")
+ }
+
+ // If there were any test failures, we want to fail the task to prevent the failures
+ // from being cached.
+ if (ext.hadFailure) {
Review Comment:
oh, you have fixed 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1747432741
##########
build.gradle:
##########
@@ -499,6 +504,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
+ ext.hadFailure = true
+ }
+ }
+
+ // This closure will copy JUnit XML files out of the sub-project's build directory and into
+ // a top-level build/junit-xml directory. This is necessary to avoid reporting on tests which
+ // were not run, but instead were restored via FROM-CACHE. See KAFKA-17479 for more details.
+ doLast {
+ if (ext.isGithubActions) {
+ def dest = rootProject.layout.buildDirectory.dir("junit-xml/${project.name}").get().asFile
+ println "Copy JUnit XML for ${project.name} to $dest"
+ ant.copy(todir: "$dest") {
+ ant.fileset(dir: "${test.reports.junitXml.entryPoint}")
+ }
+
+ // If there were any test failures, we want to fail the task to prevent the failures
+ // from being cached.
+ if (ext.hadFailure) {
Review Comment:
> Personally, I'd like to make tests stable instead of retry.
Yes, agreed. 😄 Until we get the tests in better shape, I think the retry thing is okay.
> It seems the retry issue is still existent in normal PR
Normal PR will look like:
* Any failure (flaky or not) will cause Gradle to exit with 1
* `continue-on-error: true` will mark that step green (I think... need to verify this)
* "Parse JUnit tests" will process the results correctly and exit 0 if only flaky, exit 1 if any true failures
So the overall CI workflow will be green if there are only flaky failures. Red if there are any real failures.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #17106:
URL: https://github.com/apache/kafka/pull/17106#issuecomment-2332799367
Here is how I verified this fix.
Starting with empty build cache (rm -r ~/.gradle/caches/build-cache-1), I introduced a test that always fails and ran the following gradlew command.
```
./gradlew --info --build-cache --no-scan --continue -PignoreFailures=true :metadata:test
```
The tests all run, Gradle exits with return code 1, and the following output is produced:
```
> Task :metadata:test FAILED
933 tests completed, 1 failed, 1 skipped
Finished generating test XML results (0.036 secs) into: /Users/davidarthur/Code/Apache/kafka/metadata/build/test-results/test
Generating HTML test report...
Finished generating test html results (0.043 secs) into: /Users/davidarthur/Code/Apache/kafka/metadata/build/reports/tests/test
There were failing tests. See the report at: file:///Users/davidarthur/Code/Apache/kafka/metadata/build/reports/tests/test/index.html
Copy JUnit XML for metadata to /Users/davidarthur/Code/Apache/kafka/build/junit-xml/metadata
[ant:copy] Copying 105 files to /Users/davidarthur/Code/Apache/kafka/build/junit-xml/metadata
FAILURE: Build failed with an exception.
* Where:
Build file '/Users/davidarthur/Code/Apache/kafka/build.gradle' line: 528
* What went wrong:
Execution failed for task ':metadata:test'.
> Failing this task since 'metadata:test' had test failures.
BUILD FAILED in 14s
31 actionable tasks: 7 executed, 24 up-to-date
```
Notably, we see the JUnit XML files moved to the right place and the task ending with failure.
If run again with no changes, the outcome is the same. After removing the failing test and re-running the gradlew command, I see the following:
```
> Task :metadata:test
Finished generating test XML results (0.046 secs) into: /Users/davidarthur/Code/Apache/kafka/metadata/build/test-results/test
Generating HTML test report...
Finished generating test html results (0.03 secs) into: /Users/davidarthur/Code/Apache/kafka/metadata/build/reports/tests/test
Copy JUnit XML for metadata to /Users/davidarthur/Code/Apache/kafka/build/junit-xml/metadata
[ant:copy] Copying 105 files to /Users/davidarthur/Code/Apache/kafka/build/junit-xml/metadata
Stored cache entry for task ':metadata:test' with cache key b2da6d0b8f4fbf99a090c0287dbf2f2c
BUILD SUCCESSFUL in 18s
31 actionable tasks: 6 executed, 25 up-to-date
```
The tests are all run, Gradle exits with 0, and we see the cache get populated. Now re-running the same gradlew command results in
```
> Task :metadata:test UP-TO-DATE
Custom actions are attached to task ':metadata:test'.
Build cache key for task ':metadata:test' is b2da6d0b8f4fbf99a090c0287dbf2f2c
Skipping task ':metadata:test' as it is up-to-date.
BUILD SUCCESSFUL in 810ms
31 actionable tasks: 31 up-to-date
```
No XML files moved, exit code 0.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1747126992
##########
build.gradle:
##########
@@ -499,6 +502,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
+ ext.hadFailure = true
+ }
+ }
+
+ // This closure will copy JUnit XML files out of the sub-project's build directory and into
+ // a top-level build/junit-xml directory. This is necessary to avoid reporting on tests which
+ // were not run, but instead were restored via FROM-CACHE. See KAFKA-17479 for more details.
+ //
+ // This closure will not run if a test fails and ignoreFailures is not true.
Review Comment:
It seems this comment is stale, right?
##########
build.gradle:
##########
@@ -499,6 +502,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
Review Comment:
not sure whether it works with `maxTestRetries` and `maxTestRetryFailures`? It should show "successful" if the failed test pass after retry.
maybe we can use `finalizedBy` instead. for example:
```
tasks.test {
finalizedBy("copyJunitXml")
}
tasks.register("copyJunitXml") {
doLast {
def dest = rootProject.layout.buildDirectory.dir("junit-xml/${project.name}").get().asFile
println "Copy JUnit XML for ${project.name} to $dest"
ant.copy(todir: "$dest") {
ant.fileset(dir: "${test.reports.junitXml.entryPoint}")
}
}
}
```
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1747403316
##########
build.gradle:
##########
@@ -499,6 +504,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
+ ext.hadFailure = true
+ }
+ }
+
+ // This closure will copy JUnit XML files out of the sub-project's build directory and into
+ // a top-level build/junit-xml directory. This is necessary to avoid reporting on tests which
+ // were not run, but instead were restored via FROM-CACHE. See KAFKA-17479 for more details.
+ doLast {
+ if (ext.isGithubActions) {
+ def dest = rootProject.layout.buildDirectory.dir("junit-xml/${project.name}").get().asFile
+ println "Copy JUnit XML for ${project.name} to $dest"
+ ant.copy(todir: "$dest") {
+ ant.fileset(dir: "${test.reports.junitXml.entryPoint}")
+ }
+
+ // If there were any test failures, we want to fail the task to prevent the failures
+ // from being cached.
+ if (ext.hadFailure) {
Review Comment:
It seems the retry issue is still existent in normal PR. We can't use the "retry" to convert the result from "failed" to "successful". Maybe we can remove the `-PmaxTestRetries=1 -PmaxTestRetryFailures=10` directly? Personally, I'd like to make tests stable instead of retry.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Fix ignoreFailures logic in CI workflow [3/n] [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17106:
URL: https://github.com/apache/kafka/pull/17106#discussion_r1747499455
##########
build.gradle:
##########
@@ -499,6 +504,32 @@ subprojects {
maxFailures = userMaxTestRetryFailures
}
}
+
+ // As we process results, check if there were any test failures.
+ afterSuite { desc, result ->
+ if (result.resultType == TestResult.ResultType.FAILURE) {
+ ext.hadFailure = true
+ }
+ }
+
+ // This closure will copy JUnit XML files out of the sub-project's build directory and into
+ // a top-level build/junit-xml directory. This is necessary to avoid reporting on tests which
+ // were not run, but instead were restored via FROM-CACHE. See KAFKA-17479 for more details.
+ doLast {
+ if (ext.isGithubActions) {
+ def dest = rootProject.layout.buildDirectory.dir("junit-xml/${project.name}").get().asFile
+ println "Copy JUnit XML for ${project.name} to $dest"
+ ant.copy(todir: "$dest") {
+ ant.fileset(dir: "${test.reports.junitXml.entryPoint}")
+ }
+
+ // If there were any test failures, we want to fail the task to prevent the failures
+ // from being cached.
+ if (ext.hadFailure) {
Review Comment:
> So the overall CI workflow will be green if there are only flaky failures. Red if there are any real failures.
Got it. Thanks for detailed explanation
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org