You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/04/22 04:11:24 UTC
[GitHub] [kafka] chia7712 opened a new pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
chia7712 opened a new pull request #10585:
URL: https://github.com/apache/kafka/pull/10585
The command used by our private CI is `./gradlew cleanTest xxx:test`. It does not re-run test when we use `unitTest` and `integrationTest` to replace `test`. The root cause is that we don't offer test output (`unitTest` and `integrationTest`) to `cleanTest` task and so it does not delete related test output.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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] [kafka] ijuma commented on a change in pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#discussion_r641965516
##########
File path: build.gradle
##########
@@ -449,6 +449,14 @@ subprojects {
}
}
+ // remove test output from Test, IntegrationTest and UnitTest
Review comment:
I would say "remove test output from all test types" so that this comment doesn't go stale if we add more test types.
--
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] [kafka] ijuma commented on a change in pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#discussion_r633089109
##########
File path: build.gradle
##########
@@ -380,6 +380,15 @@ subprojects {
}
}
+ cleanTest {
+ subprojects.each {
+ delete "${it.buildDir}/test-results/unitTest"
+ delete "${it.buildDir}/reports/tests/unitTest"
+ delete "${it.buildDir}/test-results/integrationTest"
+ delete "${it.buildDir}/reports/tests/integrationTest"
+ }
+ }
+
Review comment:
Is it useful for unit and integration test to use separate output dirs? Or should they all write to the same output dir as the tests command?
--
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] [kafka] ijuma commented on a change in pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#discussion_r641757745
##########
File path: build.gradle
##########
@@ -380,6 +380,15 @@ subprojects {
}
}
+ cleanTest {
+ subprojects.each {
+ delete "${it.buildDir}/test-results/unitTest"
+ delete "${it.buildDir}/reports/tests/unitTest"
+ delete "${it.buildDir}/test-results/integrationTest"
+ delete "${it.buildDir}/reports/tests/integrationTest"
+ }
+ }
+
Review comment:
How do we know that the hardcoded directories here are all correct? Is there a way to get that data from gradle instead of hardcoding?
--
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] [kafka] chia7712 commented on pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#issuecomment-841812285
BTW, this patch fixes the commands in readme (https://github.com/apache/kafka/blob/trunk/README.md#force-re-running-tests-without-code-change)
--
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] [kafka] chia7712 commented on pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#issuecomment-841812285
BTW, this patch fixes the commands in readme (https://github.com/apache/kafka/blob/trunk/README.md#force-re-running-tests-without-code-change)
--
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] [kafka] chia7712 commented on a change in pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#discussion_r641907832
##########
File path: build.gradle
##########
@@ -380,6 +380,15 @@ subprojects {
}
}
+ cleanTest {
+ subprojects.each {
+ delete "${it.buildDir}/test-results/unitTest"
+ delete "${it.buildDir}/reports/tests/unitTest"
+ delete "${it.buildDir}/test-results/integrationTest"
+ delete "${it.buildDir}/reports/tests/integrationTest"
+ }
+ }
+
Review comment:
You are right. will copy that.
--
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] [kafka] chia7712 commented on a change in pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#discussion_r633090928
##########
File path: build.gradle
##########
@@ -380,6 +380,15 @@ subprojects {
}
}
+ cleanTest {
+ subprojects.each {
+ delete "${it.buildDir}/test-results/unitTest"
+ delete "${it.buildDir}/reports/tests/unitTest"
+ delete "${it.buildDir}/test-results/integrationTest"
+ delete "${it.buildDir}/reports/tests/integrationTest"
+ }
+ }
+
Review comment:
> Is it useful for unit and integration test to use separate output dirs? Or should they all write to the same output dir as the tests command?
We need to combine the reports if unit and integration test use the same output dir. Otherwise, the following command can generate only one report.
```./gradlew cleanTest xxx:unitTest xxx:integrationTest```
Also, changing the output dir of unit/integration may introduce some compatibility issue. It seems to me removing the output dirs is simpler.
--
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] [kafka] ijuma commented on a change in pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#discussion_r633089109
##########
File path: build.gradle
##########
@@ -380,6 +380,15 @@ subprojects {
}
}
+ cleanTest {
+ subprojects.each {
+ delete "${it.buildDir}/test-results/unitTest"
+ delete "${it.buildDir}/reports/tests/unitTest"
+ delete "${it.buildDir}/test-results/integrationTest"
+ delete "${it.buildDir}/reports/tests/integrationTest"
+ }
+ }
+
Review comment:
Is it useful for unit and integration test to use separate output dirs? Or should they all write to the same output dir as the tests command?
--
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] [kafka] chia7712 commented on a change in pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10585:
URL: https://github.com/apache/kafka/pull/10585#discussion_r633090928
##########
File path: build.gradle
##########
@@ -380,6 +380,15 @@ subprojects {
}
}
+ cleanTest {
+ subprojects.each {
+ delete "${it.buildDir}/test-results/unitTest"
+ delete "${it.buildDir}/reports/tests/unitTest"
+ delete "${it.buildDir}/test-results/integrationTest"
+ delete "${it.buildDir}/reports/tests/integrationTest"
+ }
+ }
+
Review comment:
> Is it useful for unit and integration test to use separate output dirs? Or should they all write to the same output dir as the tests command?
We need to combine the reports if unit and integration test use the same output dir. Otherwise, the following command can generate only one report.
```./gradlew cleanTest xxx:unitTest xxx:integrationTest```
Also, changing the output dir of unit/integration may introduce some compatibility issue. It seems to me removing the output dirs is simpler.
--
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] [kafka] chia7712 merged pull request #10585: MINOR: cleanTest ought to remove output of unitTest task and integrat…
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10585:
URL: https://github.com/apache/kafka/pull/10585
--
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