[jira] [Comment Edited] (RATIS-247) Add a JUnit RunListener to dump all threads when test timeout

["Tsz Wo Nicholas Sze (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/RATIS-247?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16502788#comment-16502788 ] 

Tsz Wo Nicholas Sze edited comment on RATIS-247 at 6/6/18 3:29 AM:
-------------------------------------------------------------------

Thanks for reviewing the patch.
 # The maven-compiler plugin parameters is exactly the same as the default defined in <pluginManagement>. Therefore, they are redundant.
{quote}If it's not used any more, it could be removed together with the profile (there is an empty profile now).
{quote}
Sure, it is a good idea to remove also the test-patch profile.
 # The timeout exception is created by junit (org.junit.internal.runners.statements.FailOnTimeout) so that it is environment/os independent. Unfortunately, the timeout exception class is just Exception as shown below.
{code:java}
// FailOnTimeout.java
  private void throwTimeoutException(FailOnTimeout.StatementThread thread) throws Exception {
    Exception exception = new Exception(String.format("test timed out after %d milliseconds", this.fTimeout));
    exception.setStackTrace(thread.getRecordedStackTrace());
    throw exception;
  }
{code}
{quote}Or is it just to make junit version upgrade more safe?
{quote}
Yes, the code here is more robust compared to simply hardcoded the exception message prefix.

 Here is a new patch: r247_20180606.patch


was (Author: szetszwo):
Thanks for reviewing the patch.
 # The maven-compiler plugin parameters is exactly the same as the default defined in <pluginManagement>. Therefore, they are redundant.
{quote}If it's not used any more, it could be removed together with the profile (there is an empty profile now).
{quote}
Sure, it is a good idea to remove also the test-patch profile.

 # The timeout exception is created by junit (org.junit.internal.runners.statements.FailOnTimeout) so that it is environment/os independent. Unfortunately, the timeout exception class is just Exception as shown below.
{code:java}
// FailOnTimeout.java
  private void throwTimeoutException(FailOnTimeout.StatementThread thread) throws Exception {
    Exception exception = new Exception(String.format("test timed out after %d milliseconds", this.fTimeout));
    exception.setStackTrace(thread.getRecordedStackTrace());
    throw exception;
  }
{code}
{quote}Or is it just to make junit version upgrade more safe?
{quote}
Yes, the code here is more robust compared to simply hardcoded the exception message prefix.

 Here is a new patch: r247_20180606.patch

> Add a JUnit RunListener to dump all threads when test timeout
> -------------------------------------------------------------
>
>                 Key: RATIS-247
>                 URL: https://issues.apache.org/jira/browse/RATIS-247
>             Project: Ratis
>          Issue Type: Test
>            Reporter: Tsz Wo Nicholas Sze
>            Assignee: Tsz Wo Nicholas Sze
>            Priority: Minor
>         Attachments: r247_20180606.patch
>
>
> Some unit tests fail with timeout occasionally.  It would be great if it dumps all threads when test timeout.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)