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

Propagate Mojo-lookup errors and create test-case for #1150 #1160

Closed
wants to merge 1 commit into from

Conversation

HannesWell
Copy link
Contributor

This adds a test case to reproduce #1150. It succeeds if you rename the .mvn folder of the project used in the test.

The reason for the failure is that in MavenExecutionContext.executeMojo() the looked up BuildPluginManager failed to look-up the the mojo for the maven-resources-plugin:resources goal.
The cause is probably in the IComponentLookup created for that multi-module project, since without a .mvn-folder a un-rooted look-up is created that succeeds. My first guess is that some settings are missing, but on my first look I didn't find anything.

@HannesWell
Copy link
Contributor Author

That reminds me that I wanted to create a little guide about 'How to contribute a test-case'.

@github-actions
Copy link

Test Results

611 tests  +1   601 ✔️  - 2   9m 54s ⏱️ - 1m 41s
  98 suites ±0       7 💤 ±0 
  98 files   ±0       3 +3 

For more details on these failures, see this check.

Results for commit be46e68. ± Comparison against base commit 7fca6de.

@HannesWell
Copy link
Contributor Author

HannesWell commented Dec 30, 2022

From the tests it looks like we would have seen that earlier if the mojo execution errors would not have been swallowed.

@@ -331,6 +332,7 @@ private static void executeMojo(MavenSession session, MojoExecution execution, I
lookup.lookup(BuildPluginManager.class).executeMojo(session, execution);
} catch(Exception ex) {
session.getResult().addException(ex);
throw new CoreException(Status.error("Failed to execute mojo", ex));
Copy link
Member

Choose a reason for hiding this comment

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

Thats not the place to handle this, the caller has to check the result and act accordingly (e.g. m2e has helper classes to create error markers from results).

Copy link
Member

Choose a reason for hiding this comment

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

I checked this more deeply in m2e an it seems this actually is already handled at a higher level, even though in some places we return the result. It seems strange here to have both (throwing an exception and adding it to the result). So I think simply throwing an exception would be enough

@laeubi
Copy link
Member

laeubi commented Dec 31, 2022

The cause is probably in the IComponentLookup created for that multi-module project, since without a .mvn-folder a un-rooted look-up is created that succeeds. My first guess is that some settings are missing, but on my first look I didn't find anything.

I don't think so. This is most probably caused by m2e not using the right context fro creation of the MojoExecution as mentioned in other PRs because it is invalid to use the global context (MavenImpl). I'll try to see if I find where this originates.

@laeubi
Copy link
Member

laeubi commented Dec 31, 2022

merged as part of

@laeubi laeubi closed this Dec 31, 2022
@HannesWell HannesWell deleted the test1150 branch January 6, 2023 11:23
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.

2 participants