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

[MNG-6405] Fix basedir in MavenProject.deepCopy #225

Merged
merged 4 commits into from
Apr 16, 2019

Conversation

jglick
Copy link
Contributor

@jglick jglick commented Jan 3, 2019

MNG-6405 since mojohaus/flatten-maven-plugin#53 (comment) brought this up again. I have not received any hints on the proper approach to test this.


By the way

  • You have run the Core IT successfully.

Is this not already done by the Jenkinsfile? Maybe an obsolete PR template.

@jglick
Copy link
Contributor Author

jglick commented Apr 12, 2019

Ping

@rfscholte
Copy link
Contributor

Looks like this is very easy to verify with a unittest, so can you add it?

@jglick
Copy link
Contributor Author

jglick commented Apr 13, 2019

I can add a unit test if you like. I am not sure that is going to be the most convincing demonstration that the reported bug is fixed, but it is something.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, can you add the unit test as discussed above ?

@olamy
Copy link
Member

olamy commented Apr 13, 2019

IMHO it's a bit nitpicking to require test for such change...

@jglick
Copy link
Contributor Author

jglick commented Apr 15, 2019

I am looking into a test. By the way, as far as I can tell MavenProject.<init>(MavenProject) is unused, at least in this repo; the only use of deepCopy is via MavenProject.clone in MojoExecutor.executeForkedExecutions.

Failure without patch:
junit.framework.AssertionFailedError: Base directory is preserved across clone expected:<…/maven-core/target/test-classes> but was:<…/maven-core/target/test-classes/target>
	at org.apache.maven.project.MavenProjectTest.testCloneWithBaseDir(MavenProjectTest.java:188)
@@ -1207,7 +1207,8 @@ private void deepCopy( MavenProject project )
// disown the parent

// copy fields
setFile( project.getFile() );
file = project.file;
basedir = project.basedir;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that these assignments are actually unnecessary when called from clone, since the fields are already set correctly by super.clone. They would however be needed if called from MavenProject.<init>(MavenProject); since that copy constructor is apparently neither called anywhere in Maven itself nor tested, I wonder if it even works.

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Successfully merging this pull request may close these issues.

5 participants