[MCOMPILER-372] - fix test compile issue: added dependency test path for modules #27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I added an integration test case reproducing MCOMPILER-372 test case. |
|
Discarded, new pull request to follow |
|
Closed by mistake. |
(when existing) For each module : --add-reads <module name>=ALL-UNNAMED given access to classes from unnamed modules (for example JUnit classes)
rfscholte
reviewed
Jan 3, 2020
| * @param modulePathElt | ||
| * @return | ||
| */ | ||
| private File getModuleTestPathElt( File modulePathElt ) |
There was a problem hiding this comment.
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)
|
OK, on going.
Le ven. 3 janv. 2020 à 21:13, Robert Scholte <[email protected]> a
écrit :
… ***@***.**** commented on this pull request.
------------------------------
In src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
<#27 (comment)>
:
> }
}
}
+ /**
+ * 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
<#27?email_source=notifications&email_token=AEKICU6WINGDTYY24MQIFSTQ36L7LA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQUVZPQ#pullrequestreview-338255038>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEKICU7SGD5HUI6VWYOCDADQ36L7LANCNFSM4J33NR7A>
.
|
path element, if it does not succeed, guess test path element from path element
|
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 ***@***.***> a
écrit :
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
> <#27 (comment)>
> :
>
> > }
> }
> }
>
> + /**
> + * 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
> <#27?email_source=notifications&email_token=AEKICU6WINGDTYY24MQIFSTQ36L7LA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQUVZPQ#pullrequestreview-338255038>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEKICU7SGD5HUI6VWYOCDADQ36L7LANCNFSM4J33NR7A>
> .
>
|
rfscholte
reviewed
Jan 7, 2020
| { | ||
|
|
||
| // Get test path from reactor projects | ||
| File result = getModuleTestPathEltFromReactorProjects( modulePathElt ); |
There was a problem hiding this comment.
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.
|
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 :
… ***@***.**** commented on this pull request.
------------------------------
In src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
<#27 (comment)>
:
> + {
+ 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
<#27?email_source=notifications&email_token=AEKICUZCIALIZ4EDDAUUAIDQ4S763A5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQ5PQPA#pullrequestreview-339408956>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEKICU3VT4DVWVRURRAU6JTQ4S763ANCNFSM4J33NR7A>
.
|
Prj2 was not uploading test jar to local repo. Prj2 was not opening prj2 package.
|
If you only need the reactor, please never use |
|
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
<#27?email_source=notifications&email_token=AEKICU3P5UEJOLDA6WDOQWTQ4YPTRA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINT3II#issuecomment-572210593>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEKICUYAVNE2ZV2MTPIJDRTQ4YPTRANCNFSM4J33NR7A>
.
|
No guessing by artifact name.
|
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 ***@***.***> 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
> <#27?email_source=notifications&email_token=AEKICU3P5UEJOLDA6WDOQWTQ4YPTRA5CNFSM4J33NR7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINT3II#issuecomment-572210593>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEKICUYAVNE2ZV2MTPIJDRTQ4YPTRANCNFSM4J33NR7A>
> .
>
|
gnodet
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following this checklist to help us incorporate your
contribution quickly and easily:
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.
[MPH-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MPH-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
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.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.