SONARJAVA-5973 Improve testkit tool to be able to use specific dependencies for tests #5401
Conversation
leonardo-pilastri-sonarsource
left a comment
leonardo-pilastri-sonarsource
left a comment
There was a problem hiding this comment.
I think it's a very nice improvement to have! I would change the method name and also add some specific tests for the JavaCheckVerifier
| * @param jarsToRemove a collection of jar names (without extensions) to remove from the classpath | ||
| * @return the verifier configured to use the dependencies provided as arguments to change the classpath | ||
| */ | ||
| CheckVerifier withDependenciesToChange(List<String> jarsToAdd, List<String> jarsToRemove); |
There was a problem hiding this comment.
I think I'd rather have two separate methods, one to remove some jars, and the other to add them, this way we don't have the overhead of the order of parameters, and can also avoid handling null cases.
We could have something like
CheckVerifier removeJarsFromClasspath(String... jarsToRemove);
CheckVerifier addJarsToClasspath(String... jarsToAdd);
...cks-testkit/src/main/java/org/sonar/java/checks/verifier/internal/InternalCheckVerifier.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public CheckVerifier withDependenciesToChange(List<String> jarsToAdd, List<String> jarsToRemove) { | ||
| this.jarsToAdd = jarsToAdd; |
There was a problem hiding this comment.
If we want to override we should be clear about it, from the javadoc and the signature I might think that everytime I call this method I am defining more libraries to add/remove on top of previous calls to the method.
I think if we switch to removeJarsFromClasspath and addJarsToClasspath, the implementation should also change to be "cumulative", so multiple calls would produce the expected result
There was a problem hiding this comment.
Oh, yep, I've completely forgot about verifier's tests... You're right, will do!
leonardo-pilastri-sonarsource
left a comment
leonardo-pilastri-sonarsource
left a comment
There was a problem hiding this comment.
Looks good! Thanks a lot for the nice work!!
asya-vorobeva
force-pushed
the
asya/sonarjava-5973
branch
2 times, most recently
from
b2db71b to
a112404
Compare
…encies for tests
asya-vorobeva
force-pushed
the
asya/sonarjava-5973
branch
from
a112404 to
4efefcb
Compare
|
2 participants
Added CheckVerifier.withDependenciesToChange() method to configure needed dependencies to add / remove from a classpath. See SONARJAVA-5973