git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: ant git commit: Use try-with-resources and ExpectedException


Gintas,


On 14/08/18 10:14 PM, gintas@xxxxxxxxxx wrote:
> http://git-wip-us.apache.org/repos/asf/ant/blob/e648224f/src/tests/junit/org/apache/tools/ant/taskdefs/email/EmailTaskTest.java
> ----------------------------------------------------------------------
> diff --git a/src/tests/junit/org/apache/tools/ant/taskdefs/email/EmailTaskTest.java b/src/tests/junit/org/apache/tools/ant/taskdefs/email/EmailTaskTest.java
> index a77fc92..0d0c36a 100644
> --- a/src/tests/junit/org/apache/tools/ant/taskdefs/email/EmailTaskTest.java
> +++ b/src/tests/junit/org/apache/tools/ant/taskdefs/email/EmailTaskTest.java
> @@ -25,6 +25,7 @@ import org.junit.Assert;
>  import org.junit.Before;
>  import org.junit.Rule;
>  import org.junit.Test;
> +import org.junit.rules.ExpectedException;
>  
>  /**
>   * TODO : develop these testcases - the email task needs to have attributes allowing
> @@ -35,6 +36,9 @@ public class EmailTaskTest {
>      @Rule
>      public BuildFileRule buildRule = new BuildFileRule();
>  
> +    @Rule
> +    public ExpectedException thrown = ExpectedException.none();
> +
>      @Before
>      public void setUp() {
>          buildRule.configureProject("src/etc/testcases/taskdefs/email/mail.xml");
> @@ -45,14 +49,9 @@ public class EmailTaskTest {
>       */
>      @Test
>      public void test1() {
> -        try {
> -            buildRule.executeTarget("test1");
> -        } catch (BuildException e) {
> -            // assert it's the expected one
> -            if (!e.getMessage().equals("SMTP auth only possible with MIME mail")) {
> -                throw e;
> -            }
> -        }
> +        thrown.expect(BuildException.class);
> +        thrown.expectMessage("SMTP auth only possible with MIME mail");
> +        buildRule.executeTarget("test1");
>      }
>  
>      /**
> @@ -60,14 +59,9 @@ public class EmailTaskTest {
>       */
>      @Test
>      public void test2() {
> -        try {
> -            buildRule.executeTarget("test2");
> -        } catch (BuildException e) {
> -            // assert it's the expected one
> -            if (!e.getMessage().equals("SSL and STARTTLS only possible with MIME mail")) {
> -                throw e;
> -            }
> -        }
> +        thrown.expect(BuildException.class);
> +        thrown.expectMessage("SSL and STARTTLS only possible with MIME mail");
> +        buildRule.executeTarget("test2");
>      }
>  
>      /**

Could you tell me what was technically wrong with the way I had
committed it yesterday and why you felt that it had to be changed into
this form?

When I looked into this test during the last couple of days, I realized
they weren't functional and were broken. So I fixed them and used a
particular coding style that I am comfortable with. I am not a fan of
using the @Rule based expected exceptions which are stored as member
variables in the class and which then have to be setup before the actual
testing happens. To me the try/catch block is much more intuitive and
gives me more control as well as a better read of what the test case
expects. Keeping that detail aside, I decided to use a particular coding
style that I was comfortable with when adding that code. The tests are
working fine. So what was the need to override that commit with a coding
style change? Is this how you are going to continue with your future
commits?

-Jaikiran



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxx
For additional commands, e-mail: dev-help@xxxxxxxxxxxxxx