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

Order of parsed Maven modules is sometimes not correct #601

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fabapp2
Copy link
Contributor

@fabapp2 fabapp2 commented Jul 18, 2023

What's changed?

The method MavenMojoProjectParser#collectPoms adds the poms in the wrong order.
I think the problem can be fixed by using MavenSession#getProjects() instead.

What's your motivation?

Anything in particular you'd like reviewers to focus on?

I need some advice on how you think this should be tested.

My preferred approach would have been to test the method collectPoms and verify that the poms added to the paths exactly in the order as returned by MavenSession#getProjects().
I would have liked to pass in a Mock for MavenSession and verify the call.
But there are some problems with this approach:

  • The method is private, I would have been nasty and used reflection to make it accessible.
  • No Mockito is available

And an additional test that verifies the expected behavior of MavenSession#getProjects()- the correct ordering

The next (additional) way to test this I'd see is an integration test for the plugin itself and verifying the parsing events. I did not understand RewriteRunIT and would be interested in your opinion of how to approach this before proceeding.

Anyone you would like to review specifically?

@timtebeek

Have you considered any alternatives or workarounds?

By decoupling the MavenMojoProject parser from File and Maven unittest would become easier.

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@timtebeek timtebeek added the bug Something isn't working label Jul 18, 2023
@timtebeek
Copy link
Contributor

I've struggled a bit locally to see if we can test this through the integration test feature, but no luck thus far. My breakpoints aren't triggered, and even log output I add isn't showing; Perhaps you'd have more luck; my branch is here.

I'm exploring a bit the logic in SBM and the modules going into the Maven parser, roughly starting here:
https://github.com/spring-projects-experimental/spring-boot-migrator/blob/fcdfe7134e07386f255d288a6fd0dc37e98ad36b/sbm-rewrite-maven-parser/src/main/java/org/springframework/sbm/parsers/RewriteMavenProjectParser.java#L148-L153
There's a TODO comment there regarding the order, but since runPerSubmodule is false I don't think the first mavenProject argument to listSourceFiles is even being used there.

Instead we look to consistently use mavenSession.getProjects() to iterate over the modules in order returned by the session:

Map<MavenProject, List<Marker>> projectProvenances = mavenSession.getProjects().stream()
.collect(Collectors.toMap(Function.identity(), this::generateProvenance));
Map<MavenProject, Xml.Document> projectMap = parseMaven(mavenSession.getProjects(), projectProvenances, ctx);
return mavenSession.getProjects().stream()

Not quite sure how that might end up with a different order as compared to your changes here. 🤔

Will continue later, unless you have any new insights given the above.

@fabapp2
Copy link
Contributor Author

fabapp2 commented Jul 21, 2023

I understand that's a tricky part. I would think the problem will be solved with 9ffe2b3.
If you want we could see if we can use something from here.
There we create a Plexus container, call Maven and use a listener to get access to the session which can then be used to feed tests. Not straightforward but it allows testing methods depending on Maven components (that are difficult to initialize).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants