Skip to content
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

Conversation

NilsRenaud
Copy link
Contributor

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 or add-opens, the PR is still accessible here : #461

To 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.

public class AppTest
{
@Test
void testNoop()
Copy link
Contributor Author

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" ) );
Copy link
Contributor

@Tibor17 Tibor17 Feb 16, 2022

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

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.

@NilsRenaud NilsRenaud force-pushed the SUREFIRE-1909_replace_add_exports_with_add_opens branch from 9c12746 to 0f77b8a Compare February 16, 2022 14:35
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.8.1</version>
Copy link
Contributor

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 <renaud.nils@gmail.com>
@NilsRenaud NilsRenaud force-pushed the SUREFIRE-1909_replace_add_exports_with_add_opens branch from 0f77b8a to c632a9c Compare February 16, 2022 15:22
@Tibor17
Copy link
Contributor

Tibor17 commented Feb 16, 2022

@NilsRenaud
The build is running...

@Tibor17 Tibor17 merged commit 6b4ccdd into apache:master Feb 17, 2022
@NilsRenaud
Copy link
Contributor Author

Thanks @Tibor17 for your mentoring all along the way, that was great !

Since I'm around, I've 2 questions :

  • have you any insight on the 3.0 final release date ?
  • And I guess the answer will be no, but would it be possible to backport this fix on 2.22 release ? ^^'

@NilsRenaud NilsRenaud deleted the SUREFIRE-1909_replace_add_exports_with_add_opens branch February 17, 2022 08:31
@Tibor17
Copy link
Contributor

Tibor17 commented Feb 17, 2022

@NilsRenaud
I have backported many fixes to the branch release/2.22.3. Truly I wanted to cut the release 2.22.3 but hopefully I have not done it because a critical issue was fixed.
Feel free to Cherry-Pick your commit from master to the target branch and I will confirm that on GH.
Btw, I am facing one more issue where the JVM hangs, I know the root cause, and it will be pushed right after yours.

@NilsRenaud
Copy link
Contributor Author

Backport PR created targeting 2.22.3 : #473

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants