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 <jaiki...@apache.org> wrote: > Gintas, > > > On 14/08/18 10:14 PM, gin...@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-unsubscr...@ant.apache.org > For additional commands, e-mail: dev-h...@ant.apache.org > >
