[MNG-6533] Test: ProjectBuildingException miss reference to MavenProject #197

mickaelistria · 2018-11-29T17:33:58Z

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

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.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MNG-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.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the Core IT successfully.

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.

hboutemy · 2018-12-17T22:58:49Z

maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java

@@ -84,4 +87,52 @@ public void testVersionlessManagedDependency()
            // this is expected
        }
    }
+


please use common formatting: this will improve maintenance

hboutemy · 2018-12-17T22:59:21Z

maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java

 import org.apache.maven.model.building.ModelSource;

 public class ProjectBuilderTest
    extends AbstractCoreMavenComponentTestCase
 {
-    protected String getProjectsDirectory()
+    @Override
+	protected String getProjectsDirectory()


please replace tab with space

mickaelistria · 2018-12-18T08:10:36Z

@hboutemy Thanks, I updated the change according to your comments (I could have missed some style stuff though).

hboutemy · 2018-12-27T13:08:25Z

@mickaelistria I reworked the PR, creating 2 initial little refactoring commits that make the later modification a lot easier to understand IMHO: see MNG-6533-2 branch
With that 2 commits, I now can understand the main "Prefer passing the interim project in ProjectBuildingResult" commit...
One little thing that I feel is not good: catch(Exception) in the last commit. is catching InvalidArtifactRTException not sufficient?

mickaelistria · 2019-01-02T16:29:23Z

I reworked the PR, creating 2 initial little refactoring commits that make the later modification a lot easier to understand IMHO: see MNG-6533-2 branch. With that 2 commits, I now can understand the main "Prefer passing the interim project in ProjectBuildingResult" commit...

Ok, thanks.

One little thing that I feel is not good: catch(Exception) in the last commit. is catching InvalidArtifactRTException not sufficient?

IIRC, there were some cases where this wasn't sufficient and some other exceptions could happen in m2e using this API; but I don't remember how much this memory is true; so I need to double-check.
I'll give it a try with catch (InvalidArtifactRTException) and see how m2e likes it or not on next Monday hopefully. If it seems to work as expected for m2e, I'm fine with changing it. If it doesn't I'll try to write extra-unit tests for the potential issues that are not covered by this exception.

mickaelistria · 2019-01-07T18:25:32Z

@hboutemy I'm trying to get back to work on that one, but with MNG-6530, it's much harder to test it properly in m2e (I need to deal with a sequence of commits in the right order both in Maven and m2e to be able to test this issue). I'll try to make progress anyway, but it'd be much simpler to me if we can work on MNG-6530 first.

Initialize the interim project with "simple" items (ie do not build not reference parent if it's not yet in the projectIndex) and returns it when installation fails further. This give a partial validation of the file, pretty convenient in IDEs.

Sending ModelProblems allows to keep processing other pom files.

mickaelistria · 2019-01-14T10:25:56Z

@hboutemy Sorry for reporting so late. It's not an easy task and I'm not progressing very fast with this refactoring in m2e and have hard time to test things properly.
According to my last attempts, just catching ArtifactRTException as you suggested seems enough for the tests we have. I think one good approach is just to make the modification you suggest and merge it. If later I find some other cases, I'll open other issues and create new patches.
Does that seem good to you? Do you want to take care of making the change so it's your name marked on this change, which seems pretty fair ;)

hboutemy · 2019-01-14T13:32:20Z

@mickaelistria no problem
I rebased the branch on current master, fixing the conflict, then added one commit to reduce from Exception to InvalidArtifactRTException

please review and tell me what you think about it before we review on the mailing list for merge to master

mickaelistria · 2019-01-14T14:14:31Z

All good to me. I'd be happy to see this merged and then a snapshot published.
Then if I see further concerns, I'll open other tickets with narrower scope.

hboutemy · 2019-01-15T09:50:20Z

merged to master
thank you for this hard contribution: I hope this won't be the last :)

Follow up to bd25080 which missed a few tests using the above pattern

mickaelistria force-pushed the MNG-6533 branch 4 times, most recently from bf18b96 to ff2a6de Compare November 30, 2018 10:37

hboutemy reviewed Dec 17, 2018

View reviewed changes

mickaelistria force-pushed the MNG-6533 branch from ff2a6de to 7dbf6a3 Compare December 18, 2018 08:10

mickaelistria added 3 commits January 9, 2019 08:48

[MNG-6533] Test: ProjectBuildingException miss reference to MavenProject

1c3bec8

[MNG-6533] ProjectBuilder report ModelProblem instead of Exception

8d7cfaa

Sending ModelProblems allows to keep processing other pom files.

mickaelistria force-pushed the MNG-6533 branch from 7dbf6a3 to 8d7cfaa Compare January 9, 2019 07:49

hboutemy closed this Jan 15, 2019

hboutemy self-assigned this Jan 15, 2019

gnodet added a commit to gnodet/maven that referenced this pull request Nov 20, 2024

Remove verifier.getCliOptions().add( x ) pattern (apache#197)

82f5356

Follow up to bd25080 which missed a few tests using the above pattern

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-6533] Test: ProjectBuildingException miss reference to MavenProject #197

[MNG-6533] Test: ProjectBuildingException miss reference to MavenProject #197

mickaelistria commented Nov 29, 2018

hboutemy Dec 17, 2018

hboutemy Dec 17, 2018

mickaelistria commented Dec 18, 2018

hboutemy commented Dec 27, 2018 •

edited

Loading

mickaelistria commented Jan 2, 2019

mickaelistria commented Jan 7, 2019

mickaelistria commented Jan 14, 2019

hboutemy commented Jan 14, 2019

mickaelistria commented Jan 14, 2019

hboutemy commented Jan 15, 2019

[MNG-6533] Test: ProjectBuildingException miss reference to MavenProject #197

[MNG-6533] Test: ProjectBuildingException miss reference to MavenProject #197

Conversation

mickaelistria commented Nov 29, 2018

hboutemy Dec 17, 2018

Choose a reason for hiding this comment

hboutemy Dec 17, 2018

Choose a reason for hiding this comment

mickaelistria commented Dec 18, 2018

hboutemy commented Dec 27, 2018 • edited Loading

mickaelistria commented Jan 2, 2019

mickaelistria commented Jan 7, 2019

mickaelistria commented Jan 14, 2019

hboutemy commented Jan 14, 2019

mickaelistria commented Jan 14, 2019

hboutemy commented Jan 15, 2019

hboutemy commented Dec 27, 2018 •

edited

Loading