Proposal to introduce JUnit 5 in commons-numbers

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Proposal to introduce JUnit 5 in commons-numbers

Heinrich Bohne
Right now, commons-numbers is using JUnit 4.12, the last stable version
of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
4.12 for testing whether an exception is thrown apart from either using
the deprecated class ExpectedException or adding the "expected"
parameter to the Test annotation. The problem with the latter approach
is that it is impossible to ascertain where exactly in the annotated
method the exception is thrown – it could be thrown somewhere unexpected
and the test will still pass. Besides, when testing the same exception
trigger with multiple different inputs, it is impractical to create a
separate method for each test case, which would be necessary with both
aforementioned approaches.

This has led to the creation of constructs where the expected exception
is swallowed, which has been deemed undesirable
<https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419>.
Because of this, I propose to add JUnit 5 as a dependency in
commons-numbers. JUnit 5 has several "assertThrows" methods that would
solve the described dilemma.
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Gilles Sadowski-2
Hi.

Le mer. 22 mai 2019 à 18:43, Heinrich Bohne <[hidden email]> a écrit :

>
> Right now, commons-numbers is using JUnit 4.12, the last stable version
> of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> 4.12 for testing whether an exception is thrown apart from either using
> the deprecated class ExpectedException or adding the "expected"
> parameter to the Test annotation. The problem with the latter approach
> is that it is impossible to ascertain where exactly in the annotated
> method the exception is thrown – it could be thrown somewhere unexpected
> and the test will still pass. Besides, when testing the same exception
> trigger with multiple different inputs, it is impractical to create a
> separate method for each test case, which would be necessary with both
> aforementioned approaches.
>
> This has led to the creation of constructs where the expected exception
> is swallowed, which has been deemed undesirable
> <https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419>.
> Because of this, I propose to add JUnit 5 as a dependency in
> commons-numbers. JUnit 5 has several "assertThrows" methods that would
> solve the described dilemma.

+1

Gilles

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Bruno P. Kinoshita-3
In reply to this post by Heinrich Bohne
 Sounds like a good idea. I'm planning on upgrading to Junit 5 in Commons Imaging too as we have another issue to improve the current tests and how they load test files.

+1

    On Thursday, 23 May 2019, 4:43:59 am NZST, Heinrich Bohne <[hidden email]> wrote:  
 
 Right now, commons-numbers is using JUnit 4.12, the last stable version
of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
4.12 for testing whether an exception is thrown apart from either using
the deprecated class ExpectedException or adding the "expected"
parameter to the Test annotation. The problem with the latter approach
is that it is impossible to ascertain where exactly in the annotated
method the exception is thrown – it could be thrown somewhere unexpected
and the test will still pass. Besides, when testing the same exception
trigger with multiple different inputs, it is impractical to create a
separate method for each test case, which would be necessary with both
aforementioned approaches.

This has led to the creation of constructs where the expected exception
is swallowed, which has been deemed undesirable
<https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419>.
Because of this, I propose to add JUnit 5 as a dependency in
commons-numbers. JUnit 5 has several "assertThrows" methods that would
solve the described dilemma.  
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Eric Barnhill
In reply to this post by Gilles Sadowski-2
+1

On Wed, May 22, 2019 at 3:15 PM Gilles Sadowski <[hidden email]>
wrote:

> Hi.
>
> Le mer. 22 mai 2019 à 18:43, Heinrich Bohne <[hidden email]> a
> écrit :
> >
> > Right now, commons-numbers is using JUnit 4.12, the last stable version
> > of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> > 4.12 for testing whether an exception is thrown apart from either using
> > the deprecated class ExpectedException or adding the "expected"
> > parameter to the Test annotation. The problem with the latter approach
> > is that it is impossible to ascertain where exactly in the annotated
> > method the exception is thrown – it could be thrown somewhere unexpected
> > and the test will still pass. Besides, when testing the same exception
> > trigger with multiple different inputs, it is impractical to create a
> > separate method for each test case, which would be necessary with both
> > aforementioned approaches.
> >
> > This has led to the creation of constructs where the expected exception
> > is swallowed, which has been deemed undesirable
> > <
> https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419
> >.
> > Because of this, I propose to add JUnit 5 as a dependency in
> > commons-numbers. JUnit 5 has several "assertThrows" methods that would
> > solve the described dilemma.
>
> +1
>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Eitan Adler
(please make sure to CC me on replies)

+1 on this. One thing I'd like for us to avoid a mess of different junit
versions making it difficult to know which runner will be executing the
class. It would be great if we did a complete conversion and not just
introduced new syntax. I've actually done this before on a largish Java
project from JUnit3/4 to 5. It isn't hard, just a fair amount of mechanical
code changes.



On Wed, 22 May 2019 at 15:30, Eric Barnhill <[hidden email]> wrote:

> +1
>
> On Wed, May 22, 2019 at 3:15 PM Gilles Sadowski <[hidden email]>
> wrote:
>
> > Hi.
> >
> > Le mer. 22 mai 2019 à 18:43, Heinrich Bohne <[hidden email]> a
> > écrit :
> > >
> > > Right now, commons-numbers is using JUnit 4.12, the last stable version
> > > of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> > > 4.12 for testing whether an exception is thrown apart from either using
> > > the deprecated class ExpectedException or adding the "expected"
> > > parameter to the Test annotation. The problem with the latter approach
> > > is that it is impossible to ascertain where exactly in the annotated
> > > method the exception is thrown – it could be thrown somewhere
> unexpected
> > > and the test will still pass. Besides, when testing the same exception
> > > trigger with multiple different inputs, it is impractical to create a
> > > separate method for each test case, which would be necessary with both
> > > aforementioned approaches.
> > >
> > > This has led to the creation of constructs where the expected exception
> > > is swallowed, which has been deemed undesirable
> > > <
> >
> https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419
> > >.
> > > Because of this, I propose to add JUnit 5 as a dependency in
> > > commons-numbers. JUnit 5 has several "assertThrows" methods that would
> > > solve the described dilemma.
> >
> > +1
> >
> > Gilles
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>


--
Eitan Adler
Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Gilles Sadowski-2
Hi.

Le ven. 24 mai 2019 à 06:01, Eitan Adler <[hidden email]> a écrit :
>
> (please make sure to CC me on replies)
>
> +1 on this. One thing I'd like for us to avoid a mess of different junit
> versions making it difficult to know which runner will be executing the
> class. It would be great if we did a complete conversion and not just
> introduced new syntax.

+1

> I've actually done this before on a largish Java
> project from JUnit3/4 to 5. It isn't hard, just a fair amount of mechanical
> code changes.

Patch/PR welcome.

Regards,
Gilles

>
> On Wed, 22 May 2019 at 15:30, Eric Barnhill <[hidden email]> wrote:
>
> > +1
> >
> > On Wed, May 22, 2019 at 3:15 PM Gilles Sadowski <[hidden email]>
> > wrote:
> >
> > > Hi.
> > >
> > > Le mer. 22 mai 2019 à 18:43, Heinrich Bohne <[hidden email]> a
> > > écrit :
> > > >
> > > > Right now, commons-numbers is using JUnit 4.12, the last stable version
> > > > of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> > > > 4.12 for testing whether an exception is thrown apart from either using
> > > > the deprecated class ExpectedException or adding the "expected"
> > > > parameter to the Test annotation. The problem with the latter approach
> > > > is that it is impossible to ascertain where exactly in the annotated
> > > > method the exception is thrown – it could be thrown somewhere
> > unexpected
> > > > and the test will still pass. Besides, when testing the same exception
> > > > trigger with multiple different inputs, it is impractical to create a
> > > > separate method for each test case, which would be necessary with both
> > > > aforementioned approaches.
> > > >
> > > > This has led to the creation of constructs where the expected exception
> > > > is swallowed, which has been deemed undesirable
> > > > <
> > >
> > https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419
> > > >.
> > > > Because of this, I propose to add JUnit 5 as a dependency in
> > > > commons-numbers. JUnit 5 has several "assertThrows" methods that would
> > > > solve the described dilemma.
> > >
> > > +1
> > >
> > > Gilles
> > >

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Heinrich Bohne
Hello, and excuse me for the late reply, I didn't realize until now that
the online archive of the mailing list can have more than one page per
month, and since my original email didn't show up on the first page, I
was wondering whether it ever went through (I didn't subscribe to the
list, which I now regret as I am editing the header fields of this email
manually, hoping that I do it correctly and that the message will be
attributed to the correct thread).

Regarding the point about a potential mess of multiple JUnit versions:
JUnit 5 seems to provide a "Vintage" test engine to support the
execution of JUnit 3 and 4 tests alongside JUnit 5 tests. If this
Vintage test engine is added as a dependency in a Maven project in
addition to JUnit 4, then Maven will be able to run JUnit 4 tests on the
Vintage engine whilst running JUnit 5 tests on the Jupiter engine from
JUnit 5 (provided the Jupiter engine is added as a dependency). See
https://junit.org/junit5/docs/current/user-guide/#running-tests-build-maven
from the JUnit 5 user guide.

I also tried it without the Vintage engine, i.e. only JUnit 4 and the
Jupiter engine, but then, Maven would either run only the JUnit 4 tests
or only the JUnit 5 tests, or sometimes no tests at all, who knows why.
But with the Vintage engine, new tests can be written in JUnit 5 without
requiring all existing JUnit 4 tests to be migrated to JUnit 5 at once,
because, as far as I understand it, JUnit 5 tests will be run by the
Jupiter engine, JUnit 4 tests can be run by the Vintage engine until
they are migrated to JUnit 5, and the JUnit 4 dependency, it seems to
me, is only needed for compiling the JUnit 4 tests, but not for running
them.



------------------------------------------

From    Gilles Sadowski <[hidden email]>
Subject    Re: Proposal to introduce JUnit 5 in commons-numbers
Date    Sat, 25 May 2019 10:11:43 GMT

Hi.

Le ven. 24 mai 2019 à 06:01, Eitan Adler <[hidden email]> a écrit :
>
> (please make sure to CC me on replies)
>
> +1 on this. One thing I'd like for us to avoid a mess of different junit
> versions making it difficult to know which runner will be executing the
> class. It would be great if we did a complete conversion and not just
> introduced new syntax.

+1

> I've actually done this before on a largish Java
> project from JUnit3/4 to 5. It isn't hard, just a fair amount of mechanical
> code changes.

Patch/PR welcome.

Regards,
Gilles

>
> On Wed, 22 May 2019 at 15:30, Eric Barnhill <[hidden email]> wrote:
>
> > +1
> >
> > On Wed, May 22, 2019 at 3:15 PM Gilles Sadowski <[hidden email]>
> > wrote:
> >
> > > Hi.
> > >
> > > Le mer. 22 mai 2019 à 18:43, Heinrich Bohne <[hidden email]>
a

> > > écrit :
> > > >
> > > > Right now, commons-numbers is using JUnit 4.12, the last stable version
> > > > of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> > > > 4.12 for testing whether an exception is thrown apart from either using
> > > > the deprecated class ExpectedException or adding the "expected"
> > > > parameter to the Test annotation. The problem with the latter approach
> > > > is that it is impossible to ascertain where exactly in the annotated
> > > > method the exception is thrown – it could be thrown somewhere
> > unexpected
> > > > and the test will still pass. Besides, when testing the same exception
> > > > trigger with multiple different inputs, it is impractical to create a
> > > > separate method for each test case, which would be necessary with both
> > > > aforementioned approaches.
> > > >
> > > > This has led to the creation of constructs where the expected exception
> > > > is swallowed, which has been deemed undesirable
> > > > <
> > >
> > https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419
> > > >.
> > > > Because of this, I propose to add JUnit 5 as a dependency in
> > > > commons-numbers. JUnit 5 has several "assertThrows" methods that would
> > > > solve the described dilemma.
> > >
> > > +1
> > >
> > > Gilles
> > >

Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Eitan Adler
(please make sure to CC me directly on replies)

On Sun, 26 May 2019 at 17:17, Heinrich Bohne <[hidden email]> wrote:

> ... (I didn't subscribe to the list, which I now regret as I am editing
> the header fields of this email manually, hoping that I do it correctly and
> that the message will be attributed to the correct thread).
>

This is the first time I seeing this thread.


> Regarding the point about a potential mess of multiple JUnit versions:
> JUnit 5 seems to provide a "Vintage" test engine to support the execution
> of JUnit 3 and 4 tests alongside JUnit 5 tests.
>

It is precisely this that I am advocating against.   While JUnit 5 has a
method of running older tests, by leaving the older style tests around
we're producing confusion as to how it will get run or how to write new
tests. Using it as a migration tool is reasonable, but the commons are
small enough that wholesale migration is "easy".


>
>
--
Eitan Adler
Reply | Threaded
Open this post in threaded view
|

Aw: Re: Proposal to introduce JUnit 5 in commons-numbers

Sven Rathgeber

+1
 
We did the migration 4 -> 5 for various products at work. It was straightforward.
I vote for a clean cut, as well.
 
s/Before/BeforeEach/
s/After/AfterEach
.... and you are halfway through.
 
Sven
 
Gesendet: Montag, 27. Mai 2019 um 07:28 Uhr
Von: "Eitan Adler" <[hidden email]>
An: "Heinrich Bohne" <[hidden email]>
Cc: "Commons Developers List" <[hidden email]>
Betreff: Re: Proposal to introduce JUnit 5 in commons-numbers
(please make sure to CC me directly on replies)

On Sun, 26 May 2019 at 17:17, Heinrich Bohne <[hidden email]> wrote:

> ... (I didn't subscribe to the list, which I now regret as I am editing
> the header fields of this email manually, hoping that I do it correctly and
> that the message will be attributed to the correct thread).
>

This is the first time I seeing this thread.


> Regarding the point about a potential mess of multiple JUnit versions:
> JUnit 5 seems to provide a "Vintage" test engine to support the execution
> of JUnit 3 and 4 tests alongside JUnit 5 tests.
>

It is precisely this that I am advocating against. While JUnit 5 has a
method of running older tests, by leaving the older style tests around
we're producing confusion as to how it will get run or how to write new
tests. Using it as a migration tool is reasonable, but the commons are
small enough that wholesale migration is "easy".


>
>
--
Eitan Adler

 
 

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Karl Heinz Marbaise-3
In reply to this post by Heinrich Bohne
Hi,

just a small comment of mine.

to get very good support for exceptions etc. I would suggest to use
assertj which will work in JUnit 4 and Jupiter (aka 5)...

Furthermore I would not rely on a unit testing framework for assertions
which others can do better like assertj does.

Snippets from the Docs:

assertThatThrownBy(() -> { throw new Exception("boom!"); })
.isInstanceOf(Exception.class)
.hasMessageContaining("boom");

or:

assertThatExceptionOfType(IOException.class)
.isThrownBy(() -> { throw new IOException("boom!"); })
.withMessage("%s!", "boom")
.withMessageContaining("boom")
.withNoCause();

Real code example:

assertThatIllegalArgumentException()
   .isThrownBy(() -> Complex.parse(re + "," + im + ")"));
(This is the equivalent of using
@Test(expected=IllegalArgumentException.class))...
but you can also check the message very easy:

assertThatIllegalArgumentException()
   .isThrownBy(() -> Complex.parse(re + "," + im +
")")).withMessage("....");

Another code example:

current code:
Assert.assertTrue(Complex.ofCartesian(re, im).equals(Complex.parse(str)));

And this it would looks like with AssertJ:

assertThat(Complex.parse(str)).isEqualTo(Complex.ofCartesian(re, im));


Furthermore (current code):

         Assert.assertTrue(Double.isNaN(nanZero.getArgument()));
         Assert.assertTrue(Double.isNaN(zeroNaN.getArgument()));
         Assert.assertTrue(Double.isNaN(NAN.getArgument()));

via AssertJ:

         assertThat(nanZero.getArgument()).isNaN();
         assertThat(zeroNaN.getArgument()).isNaN();
         assertThat(NAN.getArgument()).isNaN();


Furthermore about  the subject for running JUnit 4 and JUnit Jupiter
(aka 5) in one build can be simply handled by just using the following
dependencies instead of JUnit 4:

   <dependencies>
     <dependency>
       <groupId>org.junit.jupiter</groupId>
       <artifactId>junit-jupiter-engine</artifactId>
       <version>5.4.2</version>
       <scope>test</scope>
     </dependency>
     <dependency>
       <groupId>org.junit.vintage</groupId>
       <artifactId>junit-vintage-engine</artifactId>
       <version>5.4.2</version>
       <scope>test</scope>
     </dependency>
   </dependencies>

That gives you the opportunity to write your Tests with JUnit 4 and also
with JUnit Jupiter aka JUnit 5. This would help to get a smooth
transition to JUnit 5.


I have created a branch
https://github.com/khmarbaise/commons-numbers/tree/JUNIT5-ASSERTJ where
you can take a look how such tests could look like and running together
JUnit 4 / JUnit Jupiter (aka JUnit 5)...also
some examples how using assertj looks like in particular for assertions
about Exceptions inlcuding the messages.

(Just as an example not meant to be a real pull request cause that is
not finished yet. This can be improved much more)...

Those tests show me the exception messages in CoseAngle need some
improvement (missing space).

* ComplexTest using assertj assertions
* CosAngleTest using junit 5 + assertj assertions..


Kind regards
Karl Heinz Marbaise

[1]:
https://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html#exception-assertion

On 22.05.19 18:34, Heinrich Bohne wrote:

> Right now, commons-numbers is using JUnit 4.12, the last stable version
> of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> 4.12 for testing whether an exception is thrown apart from either using
> the deprecated class ExpectedException or adding the "expected"
> parameter to the Test annotation. The problem with the latter approach
> is that it is impossible to ascertain where exactly in the annotated
> method the exception is thrown – it could be thrown somewhere unexpected
> and the test will still pass. Besides, when testing the same exception
> trigger with multiple different inputs, it is impractical to create a
> separate method for each test case, which would be necessary with both
> aforementioned approaches.
>
> This has led to the creation of constructs where the expected exception
> is swallowed, which has been deemed undesirable
> <https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419>.
>
> Because of this, I propose to add JUnit 5 as a dependency in
> commons-numbers. JUnit 5 has several "assertThrows" methods that would
> solve the described dilemma.
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

sebb-2-2
On Tue, 28 May 2019 at 21:44, Karl Heinz Marbaise <[hidden email]> wrote:
>
> Hi,
>
> just a small comment of mine.
>
> to get very good support for exceptions etc. I would suggest to use
> assertj which will work in JUnit 4 and Jupiter (aka 5)...

What License does it use?

> Furthermore I would not rely on a unit testing framework for assertions
> which others can do better like assertj does.
>
> Snippets from the Docs:
>
> assertThatThrownBy(() -> { throw new Exception("boom!"); })
> .isInstanceOf(Exception.class)
> .hasMessageContaining("boom");
>
> or:
>
> assertThatExceptionOfType(IOException.class)
> .isThrownBy(() -> { throw new IOException("boom!"); })
> .withMessage("%s!", "boom")
> .withMessageContaining("boom")
> .withNoCause();
>
> Real code example:
>
> assertThatIllegalArgumentException()
>    .isThrownBy(() -> Complex.parse(re + "," + im + ")"));
> (This is the equivalent of using
> @Test(expected=IllegalArgumentException.class))...
> but you can also check the message very easy:
>
> assertThatIllegalArgumentException()
>    .isThrownBy(() -> Complex.parse(re + "," + im +
> ")")).withMessage("....");
>
> Another code example:
>
> current code:
> Assert.assertTrue(Complex.ofCartesian(re, im).equals(Complex.parse(str)));

Should not be doing comparisons that way as the expected and actual
values are not shown...

> And this it would looks like with AssertJ:
>
> assertThat(Complex.parse(str)).isEqualTo(Complex.ofCartesian(re, im));

Note that the expected and actual values are the other way round from JUnit.
This could cause some confusion; care will need to be taken when converting.

Especially since there is likely to be existing code with expected and
actual the wrong way round.
I found (and fixed!) quite a lot of that in Commons code in the past;
I expect there is more.

>
> Furthermore (current code):
>
>          Assert.assertTrue(Double.isNaN(nanZero.getArgument()));
>          Assert.assertTrue(Double.isNaN(zeroNaN.getArgument()));
>          Assert.assertTrue(Double.isNaN(NAN.getArgument()));

Again, surely one would use a comparison with expected and actual?

> via AssertJ:
>
>          assertThat(nanZero.getArgument()).isNaN();
>          assertThat(zeroNaN.getArgument()).isNaN();
>          assertThat(NAN.getArgument()).isNaN();
>
>
> Furthermore about  the subject for running JUnit 4 and JUnit Jupiter
> (aka 5) in one build can be simply handled by just using the following
> dependencies instead of JUnit 4:
>
>    <dependencies>
>      <dependency>
>        <groupId>org.junit.jupiter</groupId>
>        <artifactId>junit-jupiter-engine</artifactId>
>        <version>5.4.2</version>
>        <scope>test</scope>
>      </dependency>
>      <dependency>
>        <groupId>org.junit.vintage</groupId>
>        <artifactId>junit-vintage-engine</artifactId>
>        <version>5.4.2</version>
>        <scope>test</scope>
>      </dependency>
>    </dependencies>
>
> That gives you the opportunity to write your Tests with JUnit 4 and also
> with JUnit Jupiter aka JUnit 5. This would help to get a smooth
> transition to JUnit 5.
>
>
> I have created a branch
> https://github.com/khmarbaise/commons-numbers/tree/JUNIT5-ASSERTJ where
> you can take a look how such tests could look like and running together
> JUnit 4 / JUnit Jupiter (aka JUnit 5)...also
> some examples how using assertj looks like in particular for assertions
> about Exceptions inlcuding the messages.
>
> (Just as an example not meant to be a real pull request cause that is
> not finished yet. This can be improved much more)...
>
> Those tests show me the exception messages in CoseAngle need some
> improvement (missing space).
>
> * ComplexTest using assertj assertions
> * CosAngleTest using junit 5 + assertj assertions..
>
>
> Kind regards
> Karl Heinz Marbaise
>
> [1]:
> https://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html#exception-assertion
>
> On 22.05.19 18:34, Heinrich Bohne wrote:
> > Right now, commons-numbers is using JUnit 4.12, the last stable version
> > of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> > 4.12 for testing whether an exception is thrown apart from either using
> > the deprecated class ExpectedException or adding the "expected"
> > parameter to the Test annotation. The problem with the latter approach
> > is that it is impossible to ascertain where exactly in the annotated
> > method the exception is thrown – it could be thrown somewhere unexpected
> > and the test will still pass. Besides, when testing the same exception
> > trigger with multiple different inputs, it is impractical to create a
> > separate method for each test case, which would be necessary with both
> > aforementioned approaches.
> >
> > This has led to the creation of constructs where the expected exception
> > is swallowed, which has been deemed undesirable
> > <https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419>.
> >
> > Because of this, I propose to add JUnit 5 as a dependency in
> > commons-numbers. JUnit 5 has several "assertThrows" methods that would
> > solve the described dilemma.
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Proposal to introduce JUnit 5 in commons-numbers

Karl Heinz Marbaise-3
Hi,

On 29.05.19 00:10, sebb wrote:

> On Tue, 28 May 2019 at 21:44, Karl Heinz Marbaise <[hidden email]> wrote:
>>
>> Hi,
>>
>> just a small comment of mine.
>>
>> to get very good support for exceptions etc. I would suggest to use
>> assertj which will work in JUnit 4 and Jupiter (aka 5)...
>
> What License does it use?

https://github.com/joel-costigliola/assertj-core/blob/master/LICENSE.md

Apache License
Version 2.0, January 2004
http://www.apache.org/licenses/


JUnit 5  has the following License:

Eclipse Public License - v 2.0

https://github.com/junit-team/junit5/blob/master/LICENSE.md

>
>> Furthermore I would not rely on a unit testing framework for assertions
>> which others can do better like assertj does.
>>
>> Snippets from the Docs:
>>
>> assertThatThrownBy(() -> { throw new Exception("boom!"); })
>> .isInstanceOf(Exception.class)
>> .hasMessageContaining("boom");
>>
>> or:
>>
>> assertThatExceptionOfType(IOException.class)
>> .isThrownBy(() -> { throw new IOException("boom!"); })
>> .withMessage("%s!", "boom")
>> .withMessageContaining("boom")
>> .withNoCause();
>>
>> Real code example:
>>
>> assertThatIllegalArgumentException()
>>     .isThrownBy(() -> Complex.parse(re + "," + im + ")"));
>> (This is the equivalent of using
>> @Test(expected=IllegalArgumentException.class))...
>> but you can also check the message very easy:
>>
>> assertThatIllegalArgumentException()
>>     .isThrownBy(() -> Complex.parse(re + "," + im +
>> ")")).withMessage("....");
>>
>> Another code example:
>>
>> current code:
>> Assert.assertTrue(Complex.ofCartesian(re, im).equals(Complex.parse(str)));
>
> Should not be doing comparisons that way as the expected and actual
> values are not shown...

Yes I know but this is code I found...(current state)

https://github.com/apache/commons-numbers/blob/master/commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/ComplexTest.java#L889

>
>> And this it would looks like with AssertJ:
>>
>> assertThat(Complex.parse(str)).isEqualTo(Complex.ofCartesian(re, im));
>
> Note that the expected and actual values are the other way round from JUnit.
> This could cause some confusion; care will need to be taken when converting.
>
> Especially since there is likely to be existing code with expected and
> actual the wrong way round.
> I found (and fixed!) quite a lot of that in Commons code in the past;
> I expect there is more.

In assertj there is only one way (also based on natural reading):

assertThat(actual).isEqualTo(expected);

Apart from that by using some IDE's the conversion can be done
automatically but of course the conversions have to be checked...


Kind regards
Karl Heinz Marbaise

>
>>
>> Furthermore (current code):
>>
>>           Assert.assertTrue(Double.isNaN(nanZero.getArgument()));
>>           Assert.assertTrue(Double.isNaN(zeroNaN.getArgument()));
>>           Assert.assertTrue(Double.isNaN(NAN.getArgument()));
>
> Again, surely one would use a comparison with expected and actual?
>
>> via AssertJ:
>>
>>           assertThat(nanZero.getArgument()).isNaN();
>>           assertThat(zeroNaN.getArgument()).isNaN();
>>           assertThat(NAN.getArgument()).isNaN();
>>
>>
>> Furthermore about  the subject for running JUnit 4 and JUnit Jupiter
>> (aka 5) in one build can be simply handled by just using the following
>> dependencies instead of JUnit 4:
>>
>>     <dependencies>
>>       <dependency>
>>         <groupId>org.junit.jupiter</groupId>
>>         <artifactId>junit-jupiter-engine</artifactId>
>>         <version>5.4.2</version>
>>         <scope>test</scope>
>>       </dependency>
>>       <dependency>
>>         <groupId>org.junit.vintage</groupId>
>>         <artifactId>junit-vintage-engine</artifactId>
>>         <version>5.4.2</version>
>>         <scope>test</scope>
>>       </dependency>
>>     </dependencies>
>>
>> That gives you the opportunity to write your Tests with JUnit 4 and also
>> with JUnit Jupiter aka JUnit 5. This would help to get a smooth
>> transition to JUnit 5.
>>
>>
>> I have created a branch
>> https://github.com/khmarbaise/commons-numbers/tree/JUNIT5-ASSERTJ where
>> you can take a look how such tests could look like and running together
>> JUnit 4 / JUnit Jupiter (aka JUnit 5)...also
>> some examples how using assertj looks like in particular for assertions
>> about Exceptions inlcuding the messages.
>>
>> (Just as an example not meant to be a real pull request cause that is
>> not finished yet. This can be improved much more)...
>>
>> Those tests show me the exception messages in CoseAngle need some
>> improvement (missing space).
>>
>> * ComplexTest using assertj assertions
>> * CosAngleTest using junit 5 + assertj assertions..
>>
>>
>> Kind regards
>> Karl Heinz Marbaise
>>
>> [1]:
>> https://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html#exception-assertion
>>
>> On 22.05.19 18:34, Heinrich Bohne wrote:
>>> Right now, commons-numbers is using JUnit 4.12, the last stable version
>>> of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
>>> 4.12 for testing whether an exception is thrown apart from either using
>>> the deprecated class ExpectedException or adding the "expected"
>>> parameter to the Test annotation. The problem with the latter approach
>>> is that it is impossible to ascertain where exactly in the annotated
>>> method the exception is thrown – it could be thrown somewhere unexpected
>>> and the test will still pass. Besides, when testing the same exception
>>> trigger with multiple different inputs, it is impractical to create a
>>> separate method for each test case, which would be necessary with both
>>> aforementioned approaches.
>>>
>>> This has led to the creation of constructs where the expected exception
>>> is swallowed, which has been deemed undesirable
>>> <https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419>.
>>>
>>> Because of this, I propose to add JUnit 5 as a dependency in
>>> commons-numbers. JUnit 5 has several "assertThrows" methods that would
>>> solve the described dilemma.
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: *** GMX Spamverdacht *** Aw: Re: Proposal to introduce JUnit 5 in commons-numbers

Heinrich Bohne
In reply to this post by Sven Rathgeber
So, if the migration from JUnit 4 to JUnit 5 has to happen
instantaneously, how about creating a new branch for the migration? It's
a lot of files that have to be refactored, which is going to take some
time, and since the changes will need to be reviewed, it would probably
be easier if the review can already take place while the migration is in
progress, rather than all at once when it is finished. Also, it would
reduce the risk of conflicting pull requests, thus potentially saving time.

On 5/27/19 8:28 AM, Sven Rathgeber wrote:

> +1
>
> We did the migration 4 -> 5 for various products at work. It was straightforward.
> I vote for a clean cut, as well.
>
> s/Before/BeforeEach/
> s/After/AfterEach
> .... and you are halfway through.
>
> Sven
>
> Gesendet: Montag, 27. Mai 2019 um 07:28 Uhr
> Von: "Eitan Adler" <[hidden email]>
> An: "Heinrich Bohne" <[hidden email]>
> Cc: "Commons Developers List" <[hidden email]>
> Betreff: Re: Proposal to introduce JUnit 5 in commons-numbers
> (please make sure to CC me directly on replies)
>
> On Sun, 26 May 2019 at 17:17, Heinrich Bohne <[hidden email]> wrote:
>
>> ... (I didn't subscribe to the list, which I now regret as I am editing
>> the header fields of this email manually, hoping that I do it correctly and
>> that the message will be attributed to the correct thread).
>>
> This is the first time I seeing this thread.
>
>
>> Regarding the point about a potential mess of multiple JUnit versions:
>> JUnit 5 seems to provide a "Vintage" test engine to support the execution
>> of JUnit 3 and 4 tests alongside JUnit 5 tests.
>>
> It is precisely this that I am advocating against. While JUnit 5 has a
> method of running older tests, by leaving the older style tests around
> we're producing confusion as to how it will get run or how to write new
> tests. Using it as a migration tool is reasonable, but the commons are
> small enough that wholesale migration is "easy".
>
>
>>
> --
> Eitan Adler
>
>
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: *** GMX Spamverdacht *** Aw: Re: Proposal to introduce JUnit 5 in commons-numbers

Eitan Adler
On Tue, 4 Jun 2019 at 16:09, Heinrich Bohne <[hidden email]> wrote:

> So, if the migration from JUnit 4 to JUnit 5 has to happen
> instantaneously, how about creating a new branch for the migration? It's
> a lot of files that have to be refactored, which is going to take some
> time, and since the changes will need to be reviewed, it would probably
> be easier if the review can already take place while the migration is in
> progress, rather than all at once when it is finished. Also, it would
> reduce the risk of conflicting pull requests, thus potentially saving time.
>

I don't think it has to be done instantaneously but it shouldn't drag on,
or be done "as we go". In terms of creating a branch: seems reasonable to
me. I'll happily contribute and review.

That said, the first change we need to make is to modify `pom.xml` to
support running JUnit 5 tests + the vintage runner. I don't know maven too
well but took a stab at it here:
https://github.com/apache/commons-numbers/pull/47


--
Eitan Adler