git.net

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

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


Is that the reason you moved to this style of coding? I don't plan to
use ExpectedException or that style of testing in code that I write. Are
you going to keep overriding such commits?

-Jaikiran

On 15/08/18 1:18 PM, Gintautas Grigelionis wrote:
> Jaikiran,
>
> your code allowed for false positives, too
>
> Gintas
>
> On Wed, 15 Aug 2018 at 09:11, Gintautas Grigelionis <g.grigelionis@xxxxxxxxx>
> wrote:
>
>> I believe we discussed writing tests before. It is not a matter of style,
>> but of keeping assumptions and assertions explicit.
>> You replaced a JUnit 4 assertion with some code that works, but is far
>> from being clear.
>> 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. That would have been
>> helpful in eventual migration to JUnit 5, too.
>>
>> 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