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.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 >>> >>> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
