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-7982] IT’s for transitive dependency management #379

Conversation

DidierLoiseau
Copy link
Contributor

@DidierLoiseau DidierLoiseau commented Oct 6, 2024

Main PR: apache/maven#1785

This PR provides integration tests for MNG-7982, implemented in apache/maven#1357, as well as the legacy (Maven 2/3) behavior.

Unfortunately these tests fail with Maven 4.0.0-beta-4 and the current master:

  • MavenITmng7982DependencyManagementTransitivityTest: the test shows that, currently, the transitive dependency management fails for module d, but if you comment it out, you’ll see that it works for e, which is inconsistent – dependency management is ignored for just the first level of dependencies, not deeper layers.
  • MavenITmng4720DependencyManagementExclusionMergeTest was updated to show the behavior expected after MNG-7982: exclusions from dependency management in transitive dependencies should also be picked up.

Both tests can be fixed by making MavenSessionBuilderSupplier#getDependencyManager(boolean) return a TransitiveDependencyManager when the parameter is true, without breaking any other tests.

See also my comment in MNG-5761 about this inconsistency.

If <dependencyManagement> is transitive, its exclusions should be as well.
@DidierLoiseau DidierLoiseau force-pushed the feature/MNG-7982-transitive-dependency-management branch from 64e0c07 to 771f41f Compare October 7, 2024 20:14
@@ -39,11 +40,13 @@ public MavenITmng4720DependencyManagementExclusionMergeTest() {
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I update the super() above? Maybe keep the old test for [2.0.6,4.0.0) and move these changes to a new test for [4.0.0,)?

I don’t know how this test suite is used for previous releases…

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really used anymore. At least, we branched for Maven 3.x, not sure how we'll handle things for 4.0, 4.1, etc..., but I was actually considering merging ITs into maven to ease. I don't really see any good reason not to do that.

@gnodet
Copy link
Contributor

gnodet commented Oct 8, 2024

@DidierLoiseau could you align the branch names for this PR and apache/maven#1785, this will allow running #1785 using the new ITs, else it will use master.

@DidierLoiseau
Copy link
Contributor Author

@DidierLoiseau could you align the branch names for this PR and apache/maven#1785, this will allow running #1785 using the new ITs, else it will use master.

I see you have created #384 and apache/maven#1788, so I guess this isn’t necessary anymore?

@gnodet
Copy link
Contributor

gnodet commented Oct 8, 2024

@DidierLoiseau could you align the branch names for this PR and apache/maven#1785, this will allow running #1785 using the new ITs, else it will use master.

I see you have created #384 and apache/maven#1788, so I guess this isn’t necessary anymore?

Right

@gnodet gnodet closed this Oct 8, 2024
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