Fix matching against one of multiple env-variables pointing to one JDK#148
Fix matching against one of multiple env-variables pointing to one JDK#148HannesWell wants to merge 1 commit intoapache:masterfrom
Conversation
|
@gnodet you have written that part, could you please have a look? |
| return RequirementMatcherFactory.createVersionMatcher(tcVal).matches(reqVal); | ||
| case ENV: | ||
| return reqVal.matches("(.*,|^)\\Q" + tcVal + "\\E(,.*|$)"); | ||
| return tcVal.matches("(.*,|^)\\Q" + reqVal + "\\E(,.*|$)"); |
There was a problem hiding this comment.
a unit test will be appreciated for such code
There was a problem hiding this comment.
Yes, I already wanted to add one.
But I found only one very basic test for this at all and setting up a complete new test infrastructure for this was a bit too much work for me at the moment. :/
There was a problem hiding this comment.
I had a second look and maybe it's simpler than assumed. And I oversaw the IT tests.
But I assume a unit test would be possible relatively easy.
I'm looking into it.
There was a problem hiding this comment.
unit test will be enough, I'm fine to make method package protected and simply test it,
eg. we can provide many examples by parameterized test
There was a problem hiding this comment.
I've now figured out how to setup an integration test (and handle the trickery of Windows path with back-slashes).
Setting up a unit test would probably also be possible with the adjustments you mentioned, but since the most relevant part of the logic is in the Mojo, would again need more non trivial test setup.
The arguments when matching the collected env-variables pointing to one JDK against the required one were mistakenly swapped. This had the effect that the 'env'-variable match always failed when more than one env variable points to the same JDK.
HannesWell
force-pushed
the
fix-multi-env-variable-matching
branch
from
ce0b039 to
22768cc
Compare
|
@slawekjaranowski can we proceed with this PR now that it also has integration tests? |
|
Most of the workflows are canceled due to timeouts after a very short time and a successful build. |
2 participants
The arguments when matching the collected env-variables pointing to one JDK against the required one were mistakenly swapped. This had the effect that the 'env'-variable match always failed when more than one env variable points to the same JDK.
This happens if in an environment multiple JDKs are available through structured environment variables (e.g.
JAVA_17_HOME,JAVA_21_HOME,JAVA_25_HOME) and one of them is used as 'main' JDK and therefore JAVA_HOME also points to it.As long as only one env variable points to a JDK, this error does not make a difference.
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).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.