[MCOMPILER-372] - fix test compile issue: added dependency test path for modules

[floison51]

Following this checklist to help us incorporate your contribution quickly and easily:

• [x] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
• [x] Each commit in the pull request should have a meaningful subject line and body.
• [x] Format the pull request title like [MPH-XXX] - Fixes bug in ApproximateQuantiles, where you replace MPH-XXX with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.
• [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• [x] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• [x] You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement if you are unsure please ask on the developers list.

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.

• [x] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• [x] In any other case, please file an Apache Individual Contributor License Agreement.

[floison51]

I added an integration test case reproducing MCOMPILER-372 test case. Bug root case: project2 depends on project1. Both are modularized. Test compilation of project2 fails because project2 test can acceed to project 1 module but not to project 1 test class patch (i.e. project1/target/test-classes). When this configuration is detected, project 1 test class path is added as -patchModule compiler options. All integration tests pass.

[floison51]

Discarded, new pull request to follow

[floison51]

Closed by mistake.

[floison51]

OK, on going.

Le ven. 3 janv. 2020 à 21:13, Robert Scholte [email protected] a écrit :

@rfscholte commented on this pull request.

In src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java https://github.com/apache/maven-compiler-plugin/pull/27#discussion_r362945805 :

         }

     }
 }

• /**

• * Get module test path element from module path element
  

• * @param modulePathElt
  

• * @return
  

• */
  

• private File getModuleTestPathElt( File modulePathElt )

This methods feels a bit like best guessing. It is better to loop over all the reactorProjects to find the Paths that belong to the same Maven Project (and because of that to the same module)

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/apache/maven-compiler-plugin/pull/27?email_source=notifications&email_token=AEKICU6WINGDTYY24MQIFSTQ36L7LA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQUVZPQ#pullrequestreview-338255038, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEKICU7SGD5HUI6VWYOCDADQ36L7LANCNFSM4J33NR7A .

[floison51]

Change is commited. Best regards, François

Le lun. 6 janv. 2020 à 21:03, François Loison [email protected] a écrit :

OK, on going.

Le ven. 3 janv. 2020 à 21:13, Robert Scholte [email protected] a écrit :

@rfscholte commented on this pull request.

In src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java https://github.com/apache/maven-compiler-plugin/pull/27#discussion_r362945805 :

         }

     }
 }

• /**

• * Get module test path element from module path element
  

• * @param modulePathElt
  

• * @return
  

• */
  

• private File getModuleTestPathElt( File modulePathElt )

This methods feels a bit like best guessing. It is better to loop over all the reactorProjects to find the Paths that belong to the same Maven Project (and because of that to the same module)

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/apache/maven-compiler-plugin/pull/27?email_source=notifications&email_token=AEKICU6WINGDTYY24MQIFSTQ36L7LA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQUVZPQ#pullrequestreview-338255038, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEKICU7SGD5HUI6VWYOCDADQ36L7LANCNFSM4J33NR7A .

[floison51]

Hello,

I added this "second chance" test arefact resolution for following reason: project may have a test dependency to a test artifact wich is not in reactor projects list. For better explanation, I did the modification you suggested and built then compiler plugin.

In target/it/MCOMPILER-372_modularized_testjar, there are at least 2 projects: -> prj1 : defining prj1-tests.jar containing a Prj1Test class -> prj2 : test dependency to prj1, containing Prj2Test class extending Prj1Test

Integration test is OK.

If I run clean install from * target/it/MCOMPILER-372_modularized_testjar *folder, there is a problem. mvn clean install [ERROR] COMPILATION ERROR : [INFO] ------------------------------------------------------------- [ERROR] /C:/Users/frup82455/git/loizbak/maven-compiler-plugin/target/it/MCOMPILER-372_modularized_testjar/prj2/src/test/java/prj2/Prj2Test.java:[25,12] cannot f ind symbol symbol: class Prj1Test location: package prj1 [ERROR] /C:/Users/frup82455/git/loizbak/maven-compiler-plugin/target/it/MCOMPILER-372_modularized_testjar/prj2/src/test/java/prj2/Prj2Test.java:[27,31] cannot f ind symbol symbol: class Prj1Test [INFO] 2 errors [INFO] ------------------------------------------------------------- [INFO]

[INFO] Reactor Summary for mcompiler-372 1.0-SNAPSHOT: [INFO] [INFO] mcompiler-372 ...................................... SUCCESS [ 0.374 s] [INFO] prj0-372 ........................................... SUCCESS [ 19.295 s] [INFO] prj1-372 ........................................... SUCCESS [ 2.031 s] [INFO] prj2-372 ........................................... FAILURE [ 53.307 s] [INFO] prj3-372 ........................................... SKIPPED [INFO]

[INFO] BUILD FAILURE [INFO]

It fails because getModuleTestPathElt() tries to find sibling test artefact of C:...\maven-compiler-plugin\target\it\MCOMPILER-372_modularized_testjar\prj1\target
prj1-372-1.0-SNAPSHOT.jar. Reactor project build directory is C:...\maven-compiler-plugin\target\it\MCOMPILER-372_modularized_testjar\prj1\target
classes

So method getModuleTestPathEltFromReactorProjects() doesn't match target
prj1-372-1.0-SNAPSHOT.jar and target*classes*. No test artefact is provided back and test compilation fails.

I reactivated the "double chance" method and it works fine:

Reactor Summary for mcompiler-372 1.0-SNAPSHOT: mcompiler-372 ...................................... SUCCESS [ 0.577 s] prj0-372 ........................................... SUCCESS [ 8.593 s] prj1-372 ........................................... SUCCESS [ 5.870 s] prj2-372 ........................................... SUCCESS [ 6.139 s] prj3-372 ........................................... SUCCESS [ 6.727 s]

BUILD SUCCESS

It's strange that integration doesn't excercize this test case the same as from the command prompt. I would appreciate suggestions to improve that integration test, may be 2 test artifacts resolution flavours : one with prj1\target*classes and one with prj1\target*prj1-372-1.0-SNAPSHOT.jar

Anyway, there are other case where resolution from reactor projets list wouldn't work. Assume prj2 is build from a standalone folder, prj1 would not be in reactor list.

In other work, I can't figure out how to get rid of "double chance" test artefact resolution.

Best regards, François

Le mar. 7 janv. 2020 à 19:05, Robert Scholte [email protected] a écrit :

@rfscholte commented on this pull request.

In src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java https://github.com/apache/maven-compiler-plugin/pull/27#discussion_r363877268 :

•        {
  

•            pathElements.put( entry.getKey().getAbsolutePath(), entry.getValue() );
  

•        }
  

•    }
  

• }
• /**

• * Get module test path element from module path element
  

• * @param modulePathElt
  

• * @return
  

• */
  

• private File getModuleTestPathElt( File modulePathElt )
• {

•    // Get test path from reactor projects
  

•    File result = getModuleTestPathEltFromReactorProjects( modulePathElt );
  

Can you change this to return getModuleTestPathEltFromReactorProjects( modulePathElt );

I can't think of a reason why we need the rest of the code in this method. It would mean something went wrong somewhere else.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/apache/maven-compiler-plugin/pull/27?email_source=notifications&email_token=AEKICUZCIALIZ4EDDAUUAIDQ4S763A5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQ5PQPA#pullrequestreview-339408956, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEKICU3VT4DVWVRURRAU6JTQ4S763ANCNFSM4J33NR7A .

[rfscholte]

If you only need the reactor, please never use install anymore, but just verify. And with this plugin you should only use compile, test-compile or package (in case you depend on the jar). I wonder if we should allow pulling in test-jars from outside the reactor. I know it is possible, but it is a codesmell to me, especially in the JPMS world. (and if overcomplicates the code)

[floison51]

OK, I understand why you were reluctant to adress tests jars from repository.

Back to the root cause of the problem: The simplest solution would be to setup 2 module-info.java : one for artefact, one for test artefact (with a different module name). But IDEs like Eclipse don't support 2 java files with same name, only the main module-info.java is created.

Unfortunately, this solution has side-effects on test artefacts inheritancy. If "double chance" solution (guess test artifact names) is not accepted, I don't see possibility of a fix given the single module-info.java assumption.

Moreover, surefire plugin 3.0.0-SNAPSHOT has same problem.

So I which modify my code to put test inheritancy in main modules. This is not recommended because it adds test dependencies to main module (I will have to filter them from final package).

I will also release the pull request without the "double chance" solution.

Best regards, François

Le mer. 8 janv. 2020 à 20:04, Robert Scholte [email protected] a écrit :

If you only need the reactor, please never use install anymore, but just verify. And with this plugin you should only use compile, test-compile or package (in case you depend on the jar). I wonder if we should allow pulling in test-jars from outside the reactor. I know it is possible, but it is a codesmell to me, especially in the JPMS world. (and if overcomplicates the code)

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/apache/maven-compiler-plugin/pull/27?email_source=notifications&email_token=AEKICU3P5UEJOLDA6WDOQWTQ4YPTRA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINT3II#issuecomment-572210593, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEKICUYAVNE2ZV2MTPIJDRTQ4YPTRANCNFSM4J33NR7A .

[floison51]

Just an idea:

To me, problem comes from test artifact which is not modularized. I will experiment auto-modularization of dependent test artifact (use apache-felix maven plugin). If it works, I'll tell you.

François

Le jeu. 9 janv. 2020 à 19:15, François Loison [email protected] a écrit :

OK, I understand why you were reluctant to adress tests jars from repository.

Back to the root cause of the problem: The simplest solution would be to setup 2 module-info.java : one for artefact, one for test artefact (with a different module name). But IDEs like Eclipse don't support 2 java files with same name, only the main module-info.java is created.

Unfortunately, this solution has side-effects on test artefacts inheritancy. If "double chance" solution (guess test artifact names) is not accepted, I don't see possibility of a fix given the single module-info.java assumption.

Moreover, surefire plugin 3.0.0-SNAPSHOT has same problem.

So I which modify my code to put test inheritancy in main modules. This is not recommended because it adds test dependencies to main module (I will have to filter them from final package).

I will also release the pull request without the "double chance" solution.

Best regards, François

Le mer. 8 janv. 2020 à 20:04, Robert Scholte [email protected] a écrit :

If you only need the reactor, please never use install anymore, but just verify. And with this plugin you should only use compile, test-compile or package (in case you depend on the jar). I wonder if we should allow pulling in test-jars from outside the reactor. I know it is possible, but it is a codesmell to me, especially in the JPMS world. (and if overcomplicates the code)

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/apache/maven-compiler-plugin/pull/27?email_source=notifications&email_token=AEKICU3P5UEJOLDA6WDOQWTQ4YPTRA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINT3II#issuecomment-572210593, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEKICUYAVNE2ZV2MTPIJDRTQ4YPTRANCNFSM4J33NR7A .