You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Jaikiran Pai <ja...@apache.org> on 2018/08/15 01:14:41 UTC
Re: ant git commit: Use try-with-resources and ExpectedException
Gintas,
On 14/08/18 10:14 PM, gintas@apache.org 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@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: ant git commit: Use try-with-resources and ExpectedException
Posted by Jaikiran Pai <ja...@apache.org>.
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 <ja...@apache.org> wrote:
>
>> Gintas,
>>
>>
>> On 14/08/18 10:14 PM, gintas@apache.org 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@ant.apache.org
>> For additional commands, e-mail: dev-help@ant.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: ant git commit: Use try-with-resources and ExpectedException
Posted by Jaikiran Pai <ja...@apache.org>.
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....@gmail.com>
> 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 <ja...@apache.org> wrote:
>>
>>> Gintas,
>>>
>>>
>>> On 14/08/18 10:14 PM, gintas@apache.org 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@ant.apache.org
>>> For additional commands, e-mail: dev-help@ant.apache.org
>>>
>>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: ant git commit: Use try-with-resources and ExpectedException
Posted by Gintautas Grigelionis <g....@gmail.com>.
Jaikiran,
your code allowed for false positives, too
Gintas
On Wed, 15 Aug 2018 at 09:11, Gintautas Grigelionis <g....@gmail.com>
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 <ja...@apache.org> wrote:
>
>> Gintas,
>>
>>
>> On 14/08/18 10:14 PM, gintas@apache.org 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@ant.apache.org
>> For additional commands, e-mail: dev-help@ant.apache.org
>>
>>
Re: ant git commit: Use try-with-resources and ExpectedException
Posted by Gintautas Grigelionis <g....@gmail.com>.
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 <ja...@apache.org> wrote:
> Gintas,
>
>
> On 14/08/18 10:14 PM, gintas@apache.org 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@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
>