Re: issue with junit 5 integration

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

Re: issue with junit 5 integration

Olivier Lamy
Hi
very embarrassing issue which probably worth a quick release!
Can you create a jira?
As junit 5.3.0 has just been released,  I might be happy to cut 2.22.1 very
quickly with only this fix.
others wdyt?


On Wed, 5 Sep 2018 at 06:46, Romain Manni-Bucau <[hidden email]>
wrote:

> Hi guys,
>
> Jira seems down so sending a mail.
>
> I wanted to upgrade Meecrowave to JUnit 5.3 since it is out but I realized
> the way surefire provider was developped for JUnit 5 was forcing the
> junit-platform-engine even adding it manually in the test dependencies or
> plugin dependencies.
>
> Side note: I didn't investigated other providers but I guess it is the
> exact same but the API breakage are happening less often.
>
> I therefore created a PR to fix that ->
> https://github.com/apache/maven-surefire/pull/193
>
> Note: I didn't upgrade the JUnit 5 version in the same release but it
> should probably be done too in another commit/PR.
>
> I wonder if you have release plans which could include this. In terms of
> issues I have in mind the other thing about JUnit 5 which would be great to
> add is the support for display names instead of using the class+test names
> in the logs and reports but this is less mandatory than previous one which
> fails with a NoSuchMethod error when using vintage engine.
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
>


--
Olivier Lamy
http://twitter.com/olamy | http://linkedin.com/in/olamy
Reply | Threaded
Open this post in threaded view
|

Re: issue with junit 5 integration

Dan Tran
" surefire provider forces 1.2.0" this is not good :-)  i thought surefire
auto detect first one available in classpath

On Tue, Sep 4, 2018 at 11:06 PM Romain Manni-Bucau <[hidden email]>
wrote:

> jupiter ;)
>
> junit 5 is not jupiter but platform+ engine*s* so it must detect the full
> stack and not just the default. A first step can be to detect
> platform+jupiter+engine but I guess we will get spock, cucumber etc engine
> quickly so being generic can be worth it. In my case I have vintage-engine
> - cause i have junit4 and junit5 extensions and it is broken cause vintage
> uses platform method of the 1.3.0 and the surefire provider forces 1.2.0.
>
> In my proposal, the project dependencies (likely scope test on user side or
> compile for engine/extension writers) is used and the plugin can
> override/force some dependencies if needed. Alternative surefire could get
> a specific config for that, not sure it does worth it.
>
> Hope it is clearer.
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
>
>
> Le mer. 5 sept. 2018 à 07:41, Dan Tran <[hidden email]> a écrit :
>
> > Odd, I am under impression  surefire auto detect  junit-jupiter-engine
> at
> > runtime
> >
> > am I missing something?
> >
> > -D
> >
> > On Tue, Sep 4, 2018 at 10:08 PM Romain Manni-Bucau <
> [hidden email]>
> > wrote:
> >
> > > It does _NOT_ work and as mentionned you can test on meecrowave trying
> to
> > > upgrade the version in junit module.
> > >
> > > The test classpath build ignores project and plugin dependencies. It
> > faked
> > > working until 5.3.0-RC1 cause no breakage was visible.
> > >
> > > When testing, dont forget to use jupiter, vintage engines + platform
> > stack,
> > > otherwise code is compatible with 1.2 which gets loaded. The doc
> ignores
> > > the platform and vintage engine and guess it was not tested at all
> > checking
> > > the code ;). Not a big deal since it is a "first" release but we should
> > be
> > > ablz to get it fixed quickly.
> > >
> > >
> > > Le mar. 4 sept. 2018 23:51, Olivier Lamy <[hidden email]> a écrit :
> > >
> > > > Ok perso I don't mind (it just need to be documented)
> > > > But the issue is: users are used to simply upgrade their junit
> > > dependency.
> > > >
> > > >
> > > > On Wed, 5 Sep 2018 at 07:37, Christian Stein <[hidden email]>
> > wrote:
> > > >
> > > > > No, it works with Surefire 2.22.0 and JUnit 5.3.
> > > > >
> > > > > Just add (or move) the test-runtime dependencies to the Surefire
> > plugin
> > > > > element:
> > > > >
> > > > > <build>
> > > > >    <plugins>
> > > > >       <!-- JUnit 5 requires Surefire version 2.22.0 or higher -->
> > > > >       <plugin>
> > > > >          <artifactId>maven-surefire-plugin</artifactId>
> > > > >          <version>2.22.0</version>
> > > > >          <dependencies>
> > > > >                    <dependency>
> > > > >                        <groupId>org.junit.jupiter</groupId>
> > > > >
> <artifactId>junit-jupiter-engine</artifactId>
> > > > >                        <version>5.3.0</version>
> > > > >                    </dependency>
> > > > >          </dependencies>
> > > > >       </plugin>
> > > > >    </plugins>
> > > > > </build>
> > > > >
> > > > >
> > > > > Just checking our sample builds over at JUnit 5. Which do _NOT_ do
> > this
> > > > at
> > > > > the moment.
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Sep 4, 2018 at 11:32 PM Olivier Lamy <[hidden email]>
> > wrote:
> > > > >
> > > > > > Hi
> > > > > > very embarrassing issue which probably worth a quick release!
> > > > > > Can you create a jira?
> > > > > > As junit 5.3.0 has just been released,  I might be happy to cut
> > > 2.22.1
> > > > > very
> > > > > > quickly with only this fix.
> > > > > > others wdyt?
> > > > > >
> > > > > >
> > > > > > On Wed, 5 Sep 2018 at 06:46, Romain Manni-Bucau <
> > > [hidden email]
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hi guys,
> > > > > > >
> > > > > > > Jira seems down so sending a mail.
> > > > > > >
> > > > > > > I wanted to upgrade Meecrowave to JUnit 5.3 since it is out
> but I
> > > > > > realized
> > > > > > > the way surefire provider was developped for JUnit 5 was
> forcing
> > > the
> > > > > > > junit-platform-engine even adding it manually in the test
> > > > dependencies
> > > > > or
> > > > > > > plugin dependencies.
> > > > > > >
> > > > > > > Side note: I didn't investigated other providers but I guess it
> > is
> > > > the
> > > > > > > exact same but the API breakage are happening less often.
> > > > > > >
> > > > > > > I therefore created a PR to fix that ->
> > > > > > > https://github.com/apache/maven-surefire/pull/193
> > > > > > >
> > > > > > > Note: I didn't upgrade the JUnit 5 version in the same release
> > but
> > > it
> > > > > > > should probably be done too in another commit/PR.
> > > > > > >
> > > > > > > I wonder if you have release plans which could include this. In
> > > terms
> > > > > of
> > > > > > > issues I have in mind the other thing about JUnit 5 which would
> > be
> > > > > great
> > > > > > to
> > > > > > > add is the support for display names instead of using the
> > > class+test
> > > > > > names
> > > > > > > in the logs and reports but this is less mandatory than
> previous
> > > one
> > > > > > which
> > > > > > > fails with a NoSuchMethod error when using vintage engine.
> > > > > > >
> > > > > > > Romain Manni-Bucau
> > > > > > > @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> > > > > > > <https://rmannibucau.metawerx.net/> | Old Blog
> > > > > > > <http://rmannibucau.wordpress.com> | Github <
> > > > > > > https://github.com/rmannibucau> |
> > > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > > > > > > <
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Olivier Lamy
> > > > > > http://twitter.com/olamy | http://linkedin.com/in/olamy
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Olivier Lamy
> > > > http://twitter.com/olamy | http://linkedin.com/in/olamy
> > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: issue with junit 5 integration

Stephane Nicoll
In reply to this post by Olivier Lamy
How do I reproduce this problem concretely? Having to specify a dependency
in surefire is not an option for us as we want the JUnit5 provider to be
detected automatically.

I've upgraded a Spring Boot project to use 5.3.0 and I don't see any
problem.

Thanks,
S.


On Tue, Sep 4, 2018 at 10:46 PM Romain Manni-Bucau <[hidden email]>
wrote:

> Hi guys,
>
> Jira seems down so sending a mail.
>
> I wanted to upgrade Meecrowave to JUnit 5.3 since it is out but I realized
> the way surefire provider was developped for JUnit 5 was forcing the
> junit-platform-engine even adding it manually in the test dependencies or
> plugin dependencies.
>
> Side note: I didn't investigated other providers but I guess it is the
> exact same but the API breakage are happening less often.
>
> I therefore created a PR to fix that ->
> https://github.com/apache/maven-surefire/pull/193
>
> Note: I didn't upgrade the JUnit 5 version in the same release but it
> should probably be done too in another commit/PR.
>
> I wonder if you have release plans which could include this. In terms of
> issues I have in mind the other thing about JUnit 5 which would be great to
> add is the support for display names instead of using the class+test names
> in the logs and reports but this is less mandatory than previous one which
> fails with a NoSuchMethod error when using vintage engine.
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: issue with junit 5 integration

Romain Manni-Bucau
+1

Tibor got a good point noticing that we use scope provided for some junit
artifacts which can impact the way the classpath is built.

Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>


Le mer. 5 sept. 2018 à 10:31, Christian Stein <[hidden email]> a écrit :

> On Wed, Sep 5, 2018 at 10:22 AM Romain Manni-Bucau <[hidden email]>
> wrote:
>
> > From what I saw in the code the surefire provider artifact is resolved to
> > ...
>
>
> So, specifying that artifact in your setup (explicit or implicit) will lead
> to the correct version
> to be loaded. See the integration tests in Surefire, JUnit 5 sample
> projects, Spring Boot, et al.
>
> ... therefore it ignores the project overrides without my patch.
>
>
> Which is an improvement! And Surefire needs such a logic. I did something
> similar here:
>
> https://github.com/sormuras/junit-platform-maven-plugin/blob/05b7e3ae521ccb7f71d00ccd532523a99a9b6dfc/src/main/java/de/sormuras/junit/platform/maven/plugin/Resolver.java#L87-L116
>
> So, my plan is:
>
> 1. Check meecrowave configuration if it is possible to get it running with
> 2.22.0 and 5.3
> 2. Improve Surefire
>
Reply | Threaded
Open this post in threaded view
|

Re: issue with junit 5 integration

Tibor Digana-3
In reply to this post by Dan Tran
Romain, that link in Resolver means that our Provider has transitive
dependencies. This Surefire's Platform Provider + it's dependencies. Not
the JUnit5's provider.
Adding engine with another version makes sense to me in current situation.
It's similar to what we are doing when we add junit:junit:4.12.
My question is whether the Engine should have scope=provided in Surefire's
Provider in the future and the user is forced to add it anyway in his POM.

On Wed, Sep 5, 2018 at 9:16 AM Romain Manni-Bucau <[hidden email]>
wrote:

> Me too but
>
> https://github.com/apache/maven-surefire/blob/d88ce541f3ba78a12422bdfa35c98cfb5783f9ea/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireDependencyResolver.java#L142
> only handles a single artifact whereas junit5 relies on N > 1 artifacts for
> its stack.
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <
> https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
>
>
> Le mer. 5 sept. 2018 à 08:23, Dan Tran <[hidden email]> a écrit :
>
> > " surefire provider forces 1.2.0" this is not good :-)  i thought
> surefire
> > auto detect first one available in classpath
> >
> > On Tue, Sep 4, 2018 at 11:06 PM Romain Manni-Bucau <
> [hidden email]>
> > wrote:
> >
> > > jupiter ;)
> > >
> > > junit 5 is not jupiter but platform+ engine*s* so it must detect the
> full
> > > stack and not just the default. A first step can be to detect
> > > platform+jupiter+engine but I guess we will get spock, cucumber etc
> > engine
> > > quickly so being generic can be worth it. In my case I have
> > vintage-engine
> > > - cause i have junit4 and junit5 extensions and it is broken cause
> > vintage
> > > uses platform method of the 1.3.0 and the surefire provider forces
> 1.2.0.
> > >
> > > In my proposal, the project dependencies (likely scope test on user
> side
> > or
> > > compile for engine/extension writers) is used and the plugin can
> > > override/force some dependencies if needed. Alternative surefire could
> > get
> > > a specific config for that, not sure it does worth it.
> > >
> > > Hope it is clearer.
> > >
> > > Romain Manni-Bucau
> > > @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> > > <https://rmannibucau.metawerx.net/> | Old Blog
> > > <http://rmannibucau.wordpress.com> | Github <
> > > https://github.com/rmannibucau> |
> > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > > <
> > >
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > > >
> > >
> > >
> > > Le mer. 5 sept. 2018 à 07:41, Dan Tran <[hidden email]> a écrit :
> > >
> > > > Odd, I am under impression  surefire auto detect
> junit-jupiter-engine
> > > at
> > > > runtime
> > > >
> > > > am I missing something?
> > > >
> > > > -D
> > > >
> > > > On Tue, Sep 4, 2018 at 10:08 PM Romain Manni-Bucau <
> > > [hidden email]>
> > > > wrote:
> > > >
> > > > > It does _NOT_ work and as mentionned you can test on meecrowave
> > trying
> > > to
> > > > > upgrade the version in junit module.
> > > > >
> > > > > The test classpath build ignores project and plugin dependencies.
> It
> > > > faked
> > > > > working until 5.3.0-RC1 cause no breakage was visible.
> > > > >
> > > > > When testing, dont forget to use jupiter, vintage engines +
> platform
> > > > stack,
> > > > > otherwise code is compatible with 1.2 which gets loaded. The doc
> > > ignores
> > > > > the platform and vintage engine and guess it was not tested at all
> > > > checking
> > > > > the code ;). Not a big deal since it is a "first" release but we
> > should
> > > > be
> > > > > ablz to get it fixed quickly.
> > > > >
> > > > >
> > > > > Le mar. 4 sept. 2018 23:51, Olivier Lamy <[hidden email]> a
> écrit
> > :
> > > > >
> > > > > > Ok perso I don't mind (it just need to be documented)
> > > > > > But the issue is: users are used to simply upgrade their junit
> > > > > dependency.
> > > > > >
> > > > > >
> > > > > > On Wed, 5 Sep 2018 at 07:37, Christian Stein <[hidden email]
> >
> > > > wrote:
> > > > > >
> > > > > > > No, it works with Surefire 2.22.0 and JUnit 5.3.
> > > > > > >
> > > > > > > Just add (or move) the test-runtime dependencies to the
> Surefire
> > > > plugin
> > > > > > > element:
> > > > > > >
> > > > > > > <build>
> > > > > > >    <plugins>
> > > > > > >       <!-- JUnit 5 requires Surefire version 2.22.0 or higher
> -->
> > > > > > >       <plugin>
> > > > > > >          <artifactId>maven-surefire-plugin</artifactId>
> > > > > > >          <version>2.22.0</version>
> > > > > > >          <dependencies>
> > > > > > >                    <dependency>
> > > > > > >                        <groupId>org.junit.jupiter</groupId>
> > > > > > >
> > > <artifactId>junit-jupiter-engine</artifactId>
> > > > > > >                        <version>5.3.0</version>
> > > > > > >                    </dependency>
> > > > > > >          </dependencies>
> > > > > > >       </plugin>
> > > > > > >    </plugins>
> > > > > > > </build>
> > > > > > >
> > > > > > >
> > > > > > > Just checking our sample builds over at JUnit 5. Which do _NOT_
> > do
> > > > this
> > > > > > at
> > > > > > > the moment.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Sep 4, 2018 at 11:32 PM Olivier Lamy <[hidden email]
> >
> > > > wrote:
> > > > > > >
> > > > > > > > Hi
> > > > > > > > very embarrassing issue which probably worth a quick release!
> > > > > > > > Can you create a jira?
> > > > > > > > As junit 5.3.0 has just been released,  I might be happy to
> cut
> > > > > 2.22.1
> > > > > > > very
> > > > > > > > quickly with only this fix.
> > > > > > > > others wdyt?
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, 5 Sep 2018 at 06:46, Romain Manni-Bucau <
> > > > > [hidden email]
> > > > > > >
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi guys,
> > > > > > > > >
> > > > > > > > > Jira seems down so sending a mail.
> > > > > > > > >
> > > > > > > > > I wanted to upgrade Meecrowave to JUnit 5.3 since it is out
> > > but I
> > > > > > > > realized
> > > > > > > > > the way surefire provider was developped for JUnit 5 was
> > > forcing
> > > > > the
> > > > > > > > > junit-platform-engine even adding it manually in the test
> > > > > > dependencies
> > > > > > > or
> > > > > > > > > plugin dependencies.
> > > > > > > > >
> > > > > > > > > Side note: I didn't investigated other providers but I
> guess
> > it
> > > > is
> > > > > > the
> > > > > > > > > exact same but the API breakage are happening less often.
> > > > > > > > >
> > > > > > > > > I therefore created a PR to fix that ->
> > > > > > > > > https://github.com/apache/maven-surefire/pull/193
> > > > > > > > >
> > > > > > > > > Note: I didn't upgrade the JUnit 5 version in the same
> > release
> > > > but
> > > > > it
> > > > > > > > > should probably be done too in another commit/PR.
> > > > > > > > >
> > > > > > > > > I wonder if you have release plans which could include
> this.
> > In
> > > > > terms
> > > > > > > of
> > > > > > > > > issues I have in mind the other thing about JUnit 5 which
> > would
> > > > be
> > > > > > > great
> > > > > > > > to
> > > > > > > > > add is the support for display names instead of using the
> > > > > class+test
> > > > > > > > names
> > > > > > > > > in the logs and reports but this is less mandatory than
> > > previous
> > > > > one
> > > > > > > > which
> > > > > > > > > fails with a NoSuchMethod error when using vintage engine.
> > > > > > > > >
> > > > > > > > > Romain Manni-Bucau
> > > > > > > > > @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> > > > > > > > > <https://rmannibucau.metawerx.net/> | Old Blog
> > > > > > > > > <http://rmannibucau.wordpress.com> | Github <
> > > > > > > > > https://github.com/rmannibucau> |
> > > > > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > > > > > > > > <
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Olivier Lamy
> > > > > > > > http://twitter.com/olamy | http://linkedin.com/in/olamy
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Olivier Lamy
> > > > > > http://twitter.com/olamy | http://linkedin.com/in/olamy
> > > > > >
> > > > >
> > > >
> > >
> >
>


--
Cheers
Tibor