OSDir


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

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



On 15/08/18 12:41 PM, Gintautas Grigelionis wrote:
> I believe we discussed writing tests before. It is not a matter of style,
> but of keeping assumptions and assertions explicit.
Which assumption are you talking about? Can you use the current commit
to explain it?
> You replaced a JUnit 4 assertion with some code that works, but is far from
> being clear.
Again, can you explain what you mean by that?
> There is a reason why JUnit provides specialised assert... methods and you
> could have at least used assertEquals()
> on exception message rather than rethrowing it.
Like I said, it's my preference how I write it. I don't go around
changing someone else's code just because I don't like their style of
coding. I don't appreciate that being done with mine, when there's no
technical gain achieved with the change.

>  That would have been
> helpful in eventual migration to JUnit 5, too.
JUnit 5 migration isn't related to the commit I am talking about.

-Jaikiran

>
> Gintas
>
> On Wed, 15 Aug 2018 at 03:14, Jaikiran Pai <jaikiran@xxxxxxxxxx> wrote:
>
>> 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
>>
>>


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