Skip to content

[NETBEANS-3733] Suggest surefire 2.22.0 for JUnit5 tests.#2144

Merged
sdedic merged 1 commit intoapache:masterfrom
sdedic:bugfix/NETBEANS-3733_surefire-junit5
May 18, 2020
Merged

[NETBEANS-3733] Suggest surefire 2.22.0 for JUnit5 tests.#2144
sdedic merged 1 commit intoapache:masterfrom
sdedic:bugfix/NETBEANS-3733_surefire-junit5

Conversation

@sdedic
Copy link
Copy Markdown
Member

@sdedic sdedic commented May 17, 2020

Small improvement to 073c42c: the IDE properly tests for JUnit5 in runSingleMethodEnabled, so it triggers a suggest/status line message if the project is not properly set up. But the display message and POM fix installs JUnit4+older surefire, which does not work in JUnit5 environment.

I've just improved the messages for JUnit5 case + used proper Surefire version.

@sdedic sdedic requested review from jlahoda and mcdonnell-john May 17, 2020 08:23
Copy link
Copy Markdown
Contributor

@jlahoda jlahoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks reasonable; not sure about one the logic that sets surefireVersion = SUREFIRE_VERSION_SAFE.


if (ju5 && !usingSurefire2_22()) {
surefireVersion = SUREFIRE_VERSION_SAFE_5;
} else if (usingSurefire28()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the logic here - should it be:

Suggested change
} else if (usingSurefire28()) {
} else if (!usingSurefire28()) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - thanks !

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works much better ... in 78778ee

@sdedic sdedic force-pushed the bugfix/NETBEANS-3733_surefire-junit5 branch from 0ef7a6b to 78778ee Compare May 17, 2020 12:39
@jlahoda jlahoda self-requested a review May 17, 2020 20:24
Copy link
Copy Markdown
Contributor

@jlahoda jlahoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Copy Markdown
Contributor

@mcdonnell-john mcdonnell-john left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcdonnell-john
Copy link
Copy Markdown
Contributor

Probably too late for 12.0, so lets put a milestone of 12.1 on it so its not missed later on

@mcdonnell-john mcdonnell-john added this to the 12.1 milestone May 18, 2020
@sdedic sdedic merged commit cd600b0 into apache:master May 18, 2020
@neilcsmith-net
Copy link
Copy Markdown
Member

@mcdonnell-john not sure, it's certainly an annoyance. There's a chance we're going to need one more beta, so putting 12.0 on it and leaving it to @ebarboni to make the call.

@neilcsmith-net neilcsmith-net modified the milestones: 12.1, 12.0 May 18, 2020
@neilcsmith-net neilcsmith-net requested a review from ebarboni May 18, 2020 08:15
@neilcsmith-net
Copy link
Copy Markdown
Member

OTOH, @sdedic seems to have gone for it anyway!

@sdedic
Copy link
Copy Markdown
Member Author

sdedic commented May 18, 2020

OOPS! Feel free to rollback :-/

@neilcsmith-net
Copy link
Copy Markdown
Member

@sdedic 😆 well Eric really will need to review it now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants