You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2020/08/17 15:51:28 UTC

[GitHub] [curator] tamaashu opened a new pull request #372: CURATOR-582: Migrate to jUnit 5.6

tamaashu opened a new pull request #372:
URL: https://github.com/apache/curator/pull/372


   Migrated the code from TestNG 6.14.3 to jUnit 5.6.2.
   
   Change-Id: I745eab08c1fafa8bda7f9ead73c8af2e1817d878


----------------------------------------------------------------
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] [curator] eolivelli commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-707180053


   +1
   All tests passed on Travis.
   
   I would like to merge this awesome work
   
   @cammckenzie are you okay ?


----------------------------------------------------------------
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] [curator] cammckenzie commented on a change in pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #372:
URL: https://github.com/apache/curator/pull/372#discussion_r481467437



##########
File path: curator-client/src/test/java/org/apache/curator/BasicTests.java
##########
@@ -30,15 +37,13 @@
 import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.ZooKeeper;
 import org.mockito.Mockito;
-import org.testng.Assert;
-import org.testng.annotations.Test;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 public class BasicTests extends BaseClassForTests
 {
-    @Test
+    @RepeatedIfExceptionsTest(repeats = BaseClassForTests.REPEATS)

Review comment:
       I agree that not having to annotate every test is a good idea. If this functionality can be provided by configuration at a higher level that sounds like a good approach to me.




----------------------------------------------------------------
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] [curator] cammckenzie commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-707368569


   +1
   All tests pass for me locally now. Thanks @tamaashu 


----------------------------------------------------------------
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] [curator] tamaashu commented on a change in pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #372:
URL: https://github.com/apache/curator/pull/372#discussion_r475893740



##########
File path: curator-client/src/test/java/org/apache/curator/BasicTests.java
##########
@@ -30,15 +37,13 @@
 import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.ZooKeeper;
 import org.mockito.Mockito;
-import org.testng.Assert;
-import org.testng.annotations.Test;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 public class BasicTests extends BaseClassForTests
 {
-    @Test
+    @RepeatedIfExceptionsTest(repeats = BaseClassForTests.REPEATS)

Review comment:
       There are multiple ways to repeat failed tests with jUnit5.
   Most simple is Maven's surefire plugin, which works now with jUnit5.
   But in Curator in the original solution not all of the tests have been repeated when failed. Some of them have been tried only once, some not.
   This extension provides an annotation which you can use on those tests which you want to repeat when failed, but still have the option to use @Test on those you are not interested in. I thinks this greater flexibility is valuable.
   What do you think?




----------------------------------------------------------------
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] [curator] tamaashu commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-707074235


   Yeah, somehow curator-test-zk35 doesn't find the test files in the test jars, I'm trying to figure out what is the problem.


----------------------------------------------------------------
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] [curator] eolivelli commented on a change in pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #372:
URL: https://github.com/apache/curator/pull/372#discussion_r481087222



##########
File path: curator-client/src/test/java/org/apache/curator/BasicTests.java
##########
@@ -30,15 +37,13 @@
 import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.ZooKeeper;
 import org.mockito.Mockito;
-import org.testng.Assert;
-import org.testng.annotations.Test;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 public class BasicTests extends BaseClassForTests
 {
-    @Test
+    @RepeatedIfExceptionsTest(repeats = BaseClassForTests.REPEATS)

Review comment:
       Personally (non binding) I would prefer to use the standard tools from the jUnit framework + surefire.
   So add a simply rerunFailingTests on surefire configuration is a better option.
   
   It would be better to not have flaky tests at all, but that would be a different story
   
   @Randgalt what's your opinion ?
   




----------------------------------------------------------------
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] [curator] eolivelli commented on a change in pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #372:
URL: https://github.com/apache/curator/pull/372#discussion_r481087343



##########
File path: pom.xml
##########
@@ -657,6 +674,12 @@
                     <version>${maven-bundle-plugin-version}</version>
                 </plugin>
 
+                <plugin>
+                    <groupId>org.commonjava.maven.plugins</groupId>
+                    <artifactId>directory-maven-plugin</artifactId>

Review comment:
       very good ! thank you very much !




----------------------------------------------------------------
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] [curator] eolivelli commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-706949118


   Travis failed as well


----------------------------------------------------------------
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] [curator] Randgalt commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
Randgalt commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-675559073


   Thanks for your work. This will take time to review.


----------------------------------------------------------------
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] [curator] tamaashu commented on a change in pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #372:
URL: https://github.com/apache/curator/pull/372#discussion_r475886166



##########
File path: pom.xml
##########
@@ -657,6 +674,12 @@
                     <version>${maven-bundle-plugin-version}</version>
                 </plugin>
 
+                <plugin>
+                    <groupId>org.commonjava.maven.plugins</groupId>
+                    <artifactId>directory-maven-plugin</artifactId>

Review comment:
       When I tried to build only one sub-component of Curator maven-license-plugin always failed as didn't find the file **/src/etc/header.txt**.
   This plugin helps to find the root directory of the project and than it can be used in the license plugin as **${main.basedir}/src/etc/header.txt**




----------------------------------------------------------------
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] [curator] tamaashu commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-706771341


   Moved to maven based jUnit test repetition, rebased to head.


----------------------------------------------------------------
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] [curator] eolivelli commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-712843293


   @tamaashu  merged to master branch ! thank you


----------------------------------------------------------------
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] [curator] tamaashu commented on a change in pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #372:
URL: https://github.com/apache/curator/pull/372#discussion_r484383410



##########
File path: curator-client/src/test/java/org/apache/curator/BasicTests.java
##########
@@ -30,15 +37,13 @@
 import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.ZooKeeper;
 import org.mockito.Mockito;
-import org.testng.Assert;
-import org.testng.annotations.Test;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 public class BasicTests extends BaseClassForTests
 {
-    @Test
+    @RepeatedIfExceptionsTest(repeats = BaseClassForTests.REPEATS)

Review comment:
       @Randgalt I you also prefer the maven based solution, I can change it, it's not a big deal.
   The only difference is that then we cannot define on test level which tests should re-run if fail. But I don't see it as a problem.




----------------------------------------------------------------
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] [curator] tamaashu commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-712834546


   @eolivelli do you think this PR is ready to commit? If yes, can you please commit it? Thanks.


----------------------------------------------------------------
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] [curator] eolivelli merged pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #372:
URL: https://github.com/apache/curator/pull/372


   


----------------------------------------------------------------
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] [curator] eolivelli commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-706948333


   Apart from that issue I am able to run tests from my IDE cleanly (Apache NetBeans)


----------------------------------------------------------------
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] [curator] cammckenzie commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-707330569


   @eolivelli I will run the latest tests locally and confirm.


----------------------------------------------------------------
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] [curator] cammckenzie commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
cammckenzie commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-706783955


   I'm seeing the following. Is anyone else able to run a full 'mvn test'?
   
   [INFO] Curator Examples ................................... SUCCESS [  3.687 s]
   [INFO] Curator Service Discovery Server ................... SUCCESS [  9.275 s]
   [INFO] curator-test-zk35 .................................. FAILURE [  3.085 s]
   [INFO] ------------------------------------------------------------------------
   [INFO] BUILD FAILURE
   [INFO] ------------------------------------------------------------------------
   [INFO] Total time:  43:16 min
   [INFO] Finished at: 2020-10-12T10:02:09+11:00
   [INFO] ------------------------------------------------------------------------
   [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test (default-test) on project curator-test-zk35: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test failed: org.junit.platform.commons.JUnitException: TestEngine with ID 'junit-jupiter' failed to discover tests: ClassSelector [className = 'org.apache.curator.framework.recipes.atomic.TestDistributedAtomicLong'] resolution failed: org/apache/commons/math/stat/descriptive/SummaryStatistics: org.apache.commons.math.stat.descriptive.SummaryStatistics -> [Help 1]
   


----------------------------------------------------------------
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] [curator] eolivelli commented on a change in pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #372:
URL: https://github.com/apache/curator/pull/372#discussion_r475118012



##########
File path: curator-client/src/test/java/org/apache/curator/BasicTests.java
##########
@@ -30,15 +37,13 @@
 import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.ZooKeeper;
 import org.mockito.Mockito;
-import org.testng.Assert;
-import org.testng.annotations.Test;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 public class BasicTests extends BaseClassForTests
 {
-    @Test
+    @RepeatedIfExceptionsTest(repeats = BaseClassForTests.REPEATS)

Review comment:
       Isn't there any other way of achieve this behaviour? It looks quite awkward to use this custom annotation instead of a simple '@Test'

##########
File path: pom.xml
##########
@@ -657,6 +674,12 @@
                     <version>${maven-bundle-plugin-version}</version>
                 </plugin>
 
+                <plugin>
+                    <groupId>org.commonjava.maven.plugins</groupId>
+                    <artifactId>directory-maven-plugin</artifactId>

Review comment:
       What's the purpose of this plugin?




----------------------------------------------------------------
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] [curator] tamaashu commented on pull request #372: CURATOR-582: Migrate to jUnit 5.6

Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #372:
URL: https://github.com/apache/curator/pull/372#issuecomment-707179056


   > I see the same problem on " curator-test-zk35" module
   > 
   > [INFO] ------------------------------------------------------------------------
   > [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test (default-test) on project curator-test-zk35: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test failed: org.junit.platform.commons.JUnitException: TestEngine with ID 'junit-jupiter' failed to discover tests: ClassSelector [className = 'org.apache.curator.framework.recipes.atomic.TestDistributedAtomicLong'] resolution failed: org/apache/commons/math/stat/descriptive/SummaryStatistics: org.apache.commons.math.stat.descriptive.SummaryStatistics -> [Help 1]
   > [ERROR]
   > [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
   > [ERROR] Re-run Maven using the -X switch to enable full debug logging.
   > [ERROR]
   
   Fixed


----------------------------------------------------------------
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