-
Notifications
You must be signed in to change notification settings - Fork 13
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
[MRRESOURCES-150] Ensure reproducible order in bundle #69
Conversation
@elharo @slawekjaranowski please review when you have some time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test?
I moved issue to correct jira project - https://issues.apache.org/jira/browse/MRRESOURCES-150 |
Added.
Thanks.
PR title updated. Would you like me to squash commits and force-push? |
src/test/java/org/apache/maven/plugin/resources/remote/RemoteResourcesMojoTest.java
Outdated
Show resolved
Hide resolved
I can squash .... |
@@ -51,6 +52,8 @@ | |||
import org.eclipse.aether.internal.impl.SimpleLocalRepositoryManagerFactory; | |||
import org.eclipse.aether.repository.LocalRepository; | |||
|
|||
import static org.assertj.core.api.Assertions.assertThat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These days we're using Hamcrest, not assertj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is only test dependencies .... I will not require
even more JUnit 5 doesn't use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to settle on one thing. I don't care much which one, but let's not have a random mix of libraries doing the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we didn't use any Hamcrest nor assertj until this PR
@elharo any other remarks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression is that across Maven and even more broadly across Java, but definitely within Maven hamcrest is far more common now. I've certainly seen it a lot more while working my way through all the plugins than I've seen assert4j, though they are both present. JUnit 4 already depends on Hamcrest, though that got removed in JUnit 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok we have Hamcrest so I would like to merge
@adoroszlai thanks |
Thanks @elharo, @slawekjaranowski for the review. |
What changes were proposed in this pull request?
remote-resources:bundle
creates output that may not be reproducible.maven-remote-resources-plugin/src/main/java/org/apache/maven/plugin/resources/remote/BundleRemoteResourcesMojo.java
Lines 108 to 122 in 7a40047
DirectoryScanner
usesjava.io.File.list()
, which does not guarantee order.https://issues.apache.org/jira/browse/MRESOURCES-311
How was this patch tested?
Built the plugin and used it in another project. Changed
naturalOrder
toreverseOrder
, and repeated. Verified output is reversed between the two runs.