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

[SUREFIRE-2210] - Restore ordering of additional class path elements #686

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

dmlloyd
Copy link
Contributor

@dmlloyd dmlloyd commented Nov 10, 2023

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 [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace SUREFIRE-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 install to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean install).

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.

--

The recent change (47eb197, since 3.2.0) to support more additional class path items during test has broken the use case of testing multi-release projects where ordering of class path elements is significant by changing the additional class path items collection from a List to a HashSet, which has unreliable ordering semantics. This simple fix preserves the original class path order specified in the POM by using a LinkedHashSet, which preserves insertion order, instead of a HashSet, which does not.

/cc @kwin

@dmlloyd
Copy link
Contributor Author

dmlloyd commented Nov 13, 2023

I've added a test, however it's not clear that this will always detect a regression. The reason is that the problem was caused by the unpredictable iteration order of HashSet; it's possible that the iteration order of HashSet is coincidentally correct for a given set, thus it's difficult to detect the error with 100% certainty. Still, having the test may catch a regression someday.

@kwin
Copy link
Member

kwin commented Nov 13, 2023

@dmlloyd Looks good to me. Thanks. Just one minor stylistic remark.

@dmlloyd
Copy link
Contributor Author

dmlloyd commented Nov 13, 2023

Pushed up a fix to a funny little class loader bug relating to canonicalization of paths when using ClassLoader#getResources with an absolute path name. I'm sure it's fine...

@kwin kwin merged commit 47c5816 into apache:master Nov 14, 2023
12 of 13 checks passed
@dmlloyd dmlloyd deleted the surefire-2210 branch November 14, 2023 12:41
@dmlloyd
Copy link
Contributor Author

dmlloyd commented Nov 28, 2023

Is there some procedure to request a release of Surefire with this fix? @Tibor17 are you the person to ask?

Thanks...

@dmlloyd
Copy link
Contributor Author

dmlloyd commented Nov 28, 2023

/cc @slawekjaranowski, could I please request a release including this fix? Thank you!

@kwin
Copy link
Member

kwin commented Nov 28, 2023

The mailing list is the proper place to ask for a release.

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