Jaikiran, your code allowed for false positives, too
Gintas On Wed, 15 Aug 2018 at 09:11, Gintautas Grigelionis <g.grigelio...@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 <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 >> >>
