-
Notifications
You must be signed in to change notification settings - Fork 135
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
[MRELEASE-1087] Upgrade Maven to 3.2.5 (and de-plexus) #118
Conversation
the GenerateReleasePomsPhaseTest
While we figure out what is with this UT specifically.
I am aware this PR is really BIG and nothing else describes it fully than vague "catch up", but we need to perform this jump, as current master is really lagging and IMHO has way too much debt than would be somewhat IMHO acceptable. |
There is one problem: all UT and IT pass OK except this one that is disabled (hence build is green): GenerateReleasePomsPhaseTest I have hard time even to understand what this UT is about, and even less clues WHY it fails, seems it has to do how POM is persisted and some change probably between 3.0 and 3.2 "ways of doing it". So here I'd expect some explanation and help from @hboutemy @rfscholte and @michael-o to figure it out. |
Will try to go through today/tomorrow. |
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
Show resolved
Hide resolved
...r/src/main/java/org/apache/maven/shared/release/config/PropertiesReleaseDescriptorStore.java
Show resolved
Hide resolved
...r/src/main/java/org/apache/maven/shared/release/config/PropertiesReleaseDescriptorStore.java
Outdated
Show resolved
Hide resolved
...se-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java
Show resolved
Hide resolved
we need a MRELEASE Jira issue with an update of every commit message to be able to track the change in the future |
|
// FIXME ... setDependencyArtifacts - set direct dependencies ... | ||
// but in org.apache.maven.shared.release.phase.GenerateReleasePomsPhase.createReleaseDependencies | ||
// getArtifacts is used ... | ||
|
||
// so we probably also need call setResolvedArtifacts | ||
|
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.
Probable cause of ignore tests in AbstractRewritingReleasePhaseTestCase
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.
For example, I have hard time to understand why release needs to resolve anything at all...
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.
And why we need use setDependencyArtifacts
directly ...
code is also strange for me we check:
if ( project.getDependencyArtifacts() == null )
and next on different object
resolvedProject.setDependencyArtifacts(....)
I will check what happens if I call
resolvedProject.setResolvedArtifacts( resolvedProject.getDependencyArtifacts() )
of course I don't understand it very well
Jira issue created https://issues.apache.org/jira/browse/MRELEASE-1087 |
I updated/fixed things reported so far, @slawekjaranowski did re-enable the failing UT GenerateReleasePomsPhaseTest (while he did disable several tests in it's parent class), but also there is that DefaultVersionInfoTest as well... So, I'd like to see some consensus about these changes and finally make this PR merged. |
Cool, thanks @slawekjaranowski ! |
@rfscholte or @hboutemy would be good if any of you would bless this PR so we can merge. Again, as before, if there is any other PR scheduled, please merge it BEFORE this one, I will handle conflicts. But, this PR clearly shows severe issues with m-release-p:
All in all, I still think what I said before: "this plugin does WAAY more than it should", and hence, is very fragile (the plugin but it's process as well). Anyway, let's move on, and we can tinker more about these later. |
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.
some nit comments/question
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
Outdated
Show resolved
Hide resolved
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
Show resolved
Hide resolved
...manager/src/main/java/org/apache/maven/shared/release/phase/AbstractInputVariablesPhase.java
Outdated
Show resolved
Hide resolved
...manager/src/main/java/org/apache/maven/shared/release/phase/AbstractInputVariablesPhase.java
Show resolved
Hide resolved
* aligned logger uses (using pattern from maven core MNG-7278) * fixed Arrays FQCN
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.
LGTM: a good pass at cleanup that will prepare for next ones
I hope M6 will be the last milestone and we can after go to final
I assume that SCM 2 will be in MRELEASE 4 only, no? |
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.
ship it!
Mass changes to plugin:
Two UTs are Ignored for now
All but two UTs pass that I cannot interpret:
maven-release/maven-release-manager/src/test/java/org/apache/maven/shared/release/versions/DefaultVersionInfoTest.java
Lines 235 to 239 in 85d973c
https://github.com/apache/maven-release/blob/85d973c87724aa2bc0698e6773dd9ce2a0fc8701/maven-release-manager/src/test/java/org/apache/maven/shared/release/phase/GenerateReleasePomsPhaseTest.java
Former currently does not even run (maven version != 3.0), while latter is most probably due bad diff, probably diff logic in UT needs more "no change" detection.
All IT pass OK: