-
Notifications
You must be signed in to change notification settings - Fork 542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SUREFIRE-1909] - Support JUnit 5 reflection access by changing add-exports to add-opens #471
[SUREFIRE-1909] - Support JUnit 5 reflection access by changing add-exports to add-opens #471
Conversation
public class AppTest | ||
{ | ||
@Test | ||
void testNoop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the point of the test : to run package private test methods as suggested in JUnit 5 user guide
{ | ||
public static void main( String... args ) | ||
{ | ||
System.out.println( "module path => " + System.getProperty( "jdk.module.path" ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NilsRenaud
So, these lines are used as a debug info?
You can maybe utilize it and verify the classpath in one line, and modulapath in the second line via combining Java Hamcrest matchers.
Example:
validator.assertThatLogLine( containsString( "Running jiras.surefire1082.Jira1082Test" ), is( 1 ) );
Example with matching one line: allOf( containsString( "jar1" ), containsString( "jar2" ), ... )
The second argument is(1)
only means the number of lines matching this criteria and it is 1 in this IT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shamelessly copied an existing test with this content.
They are not useful because the java
arg file content is displayed anyway. And I'm not really sure what I should verify with these paths.
So I rather like to delete these debug lines. WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After you have deleted it, we can force the build to run.
9c12746
to
0f77b8a
Compare
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-compiler-plugin</artifactId> | ||
<version>3.8.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use the latest version because we fixed some bugs and it would be nice to come over those in Surefire too.
…ts to add-opens Signed-off-by: Nils Renaud <[email protected]>
0f77b8a
to
c632a9c
Compare
@NilsRenaud |
Thanks @Tibor17 for your mentoring all along the way, that was great ! Since I'm around, I've 2 questions :
|
@NilsRenaud |
Backport PR created targeting 2.22.3 : #473 |
This PR fixes : https://issues.apache.org/jira/browse/SUREFIRE-1909
We decided to go with the drop in replacement of
--add-exports
with--add-opens
, as suggested in the issue.To add to the background of this issue, a previous PR has been created, introducing the option to use either
add-exports
oradd-opens
, the PR is still accessible here : #461To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.