You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/07/09 07:08:59 UTC
[GitHub] [iceberg] nastra opened a new pull request #2797: Upgrade to JUnit 5
nastra opened a new pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#issuecomment-877455455
It all looks good to me and tests are passing. I just have one minor comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#issuecomment-877393876
@nastra, what are you planning to do with extensions?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#discussion_r667748360
##########
File path: build.gradle
##########
@@ -285,6 +286,7 @@ project(':iceberg-aws') {
testCompile("com.adobe.testing:s3mock-junit4") {
exclude module: "spring-boot-starter-logging"
exclude module: "logback-classic"
+ exclude group: 'junit'
Review comment:
that doesn't work unfortunately as `junit-vintage-engine` pulls in `junit` itself
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#issuecomment-877138901
@nastra just to double check, this doesn't change anything in terms of test order, log messages, etc it only enables us to use the new junit. Correct?
What does mixed junit4 and junit5 look like? Can we have a module or class w/ annotations from both versions? Is there any danger in that? Would the recommendation be to use junit5 on new tests?
cc @rdblue @aokolnychyi @RussellSpitzer
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#discussion_r667210062
##########
File path: build.gradle
##########
@@ -285,6 +286,7 @@ project(':iceberg-aws') {
testCompile("com.adobe.testing:s3mock-junit4") {
exclude module: "spring-boot-starter-logging"
exclude module: "logback-classic"
+ exclude group: 'junit'
Review comment:
Should this be global? It seems like we exclude it quite a bit and could easily have it creep back into the classpath.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#issuecomment-877027983
@rdblue can you review this one please?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#issuecomment-877438116
We're planning to use extensions so that we can start/stop a nessie server
in tests without adding Quarkus dependencies to the project. Right now this
is solved via Gradle Plugins in Nessie itself but it's a bit of a pain to
maintain it this way.
On Fri, Jul 9, 2021, 20:58 Ryan Blue ***@***.***> wrote:
> @nastra <https://github.com/nastra>, what are you planning to do with
> extensions?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/iceberg/pull/2797#issuecomment-877393876>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AACCFNIUJRUCJKBTAPCLK6LTW5BEHANCNFSM5ACF264A>
> .
>
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #2797: Upgrade to JUnit 5
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2797:
URL: https://github.com/apache/iceberg/pull/2797#issuecomment-877219901
> @nastra just to double check, this doesn't change anything in terms of test order, log messages, etc it only enables us to use the new junit. Correct?
There shouldn't be any real differences in terms of test execution or the like. As stated in the PR description, `junit-vintage-engine` is what's responsible for running JUnit 4 compatible tests, which uses **4.13**. One limitation that's worth mentioning is probably the limited support for JUnit4 native rules as stated [here](https://junit.org/junit5/docs/current/user-guide/#migrating-from-junit4-rule-support) but I don't think that this affects the Iceberg project as I haven't seen any custom Rules in the project.
> What does mixed junit4 and junit5 look like? Can we have a module or class w/ annotations from both versions? Is there any danger in that? Would the recommendation be to use junit5 on new tests?
I haven't really tried a mixed scenario of JUnit4 & JUnit5 within the same class and I would assume that one would rather fully write either JUnit4 or JUnit5 code within the same class. I would probably recommend writing JUnit5 style tests for new code, but it doesn't really matter, as people can still just write JUnit4 code as they were used to.
Having people write JUnit5 tests for new stuff would probably only matter if the project at some point would like to migrate all existing JUnit4 tests to JUnit5, but I don't think that will happen in the near future.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org