subject:"\[GitHub\] \[maven\-enforcer\] psiroky commented on a diff in pull request #213\: \[MENFORCER\-445\] Include JAVA_HOME location in the Java rule failure messages"

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

2022-12-30 Thread GitBox



psiroky commented on code in PR #213:
URL: https://github.com/apache/maven-enforcer/pull/213#discussion_r1059372306


##
enforcer-rules/src/main/java/org/apache/maven/plugins/enforcer/RequireJavaVersion.java:
##
@@ -76,6 +76,7 @@ public void execute(EnforcerRuleHelper helper) throws 
EnforcerRuleException {
 + " Build: " + detectedJdkVersion.getBuildNumber() + " 
Qualifier: "
 + detectedJdkVersion.getQualifier());
 
+super.setMessage(prepareMessage(detectedJdkVersion, getVersion()));

Review Comment:
   No, definitely not intentional. Just and oversight.
   
   Would it make sense to override the message only if it has not been provided 
by the rule configuration? Otherwise I am not sure how else would we get the 
additional info in.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

2022-12-30 Thread GitBox



psiroky commented on code in PR #213:
URL: https://github.com/apache/maven-enforcer/pull/213#discussion_r1059372306


##
enforcer-rules/src/main/java/org/apache/maven/plugins/enforcer/RequireJavaVersion.java:
##
@@ -76,6 +76,7 @@ public void execute(EnforcerRuleHelper helper) throws 
EnforcerRuleException {
 + " Build: " + detectedJdkVersion.getBuildNumber() + " 
Qualifier: "
 + detectedJdkVersion.getQualifier());
 
+super.setMessage(prepareMessage(detectedJdkVersion, getVersion()));

Review Comment:
   Hmm. So would it make sense to override the message only if it has not been 
provided by the rule configuration? Otherwise I am not sure how else would we 
get the additional info in.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

2022-12-30 Thread GitBox



psiroky commented on code in PR #213:
URL: https://github.com/apache/maven-enforcer/pull/213#discussion_r1059372306


##
enforcer-rules/src/main/java/org/apache/maven/plugins/enforcer/RequireJavaVersion.java:
##
@@ -76,6 +76,7 @@ public void execute(EnforcerRuleHelper helper) throws 
EnforcerRuleException {
 + " Build: " + detectedJdkVersion.getBuildNumber() + " 
Qualifier: "
 + detectedJdkVersion.getQualifier());
 
+super.setMessage(prepareMessage(detectedJdkVersion, getVersion()));

Review Comment:
   Hmm. So would it make sense to override the message only if it has not been 
provided by the rule configuration? Otherwise I am not sure how else would be 
get the additional info in.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

2022-12-30 Thread GitBox



psiroky commented on code in PR #213:
URL: https://github.com/apache/maven-enforcer/pull/213#discussion_r1059372306


##
enforcer-rules/src/main/java/org/apache/maven/plugins/enforcer/RequireJavaVersion.java:
##
@@ -76,6 +76,7 @@ public void execute(EnforcerRuleHelper helper) throws 
EnforcerRuleException {
 + " Build: " + detectedJdkVersion.getBuildNumber() + " 
Qualifier: "
 + detectedJdkVersion.getQualifier());
 
+super.setMessage(prepareMessage(detectedJdkVersion, getVersion()));

Review Comment:
   Hmm. So would it make sense to override the message only if it has been not 
provided by the rule configuration? Otherwise I am not sure how else would be 
get the additional info in.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

2022-12-29 Thread GitBox



psiroky commented on code in PR #213:
URL: https://github.com/apache/maven-enforcer/pull/213#discussion_r1059280780


##
enforcer-rules/src/test/java/org/apache/maven/plugins/enforcer/TestRequireJavaVersion.java:
##
@@ -89,30 +88,36 @@ void 
settingsTheJavaVersionAsNormalizedVersionShouldNotFail() throws EnforcerRul
 
 @Test
 void excludingTheCurrentJavaVersionViaRangeThisShouldFailWithException() {
-assertThrows(EnforcerRuleException.class, () -> {
-String thisVersion = 
RequireJavaVersion.normalizeJDKVersion(SystemUtils.JAVA_VERSION);
-
-RequireJavaVersion rule = new RequireJavaVersion();
-// exclude this version
-rule.setVersion("(" + thisVersion);
-
-EnforcerRuleHelper helper = EnforcerTestUtils.getHelper();
-rule.execute(helper);
-// intentionally no assertThat(...) because we expect and 
exception.
-});
-// intentionally no assertThat(...) because we expect and exception.
+String thisVersion = 
RequireJavaVersion.normalizeJDKVersion(SystemUtils.JAVA_VERSION);
+String requiredVersion = "(" + thisVersion;
+
+assertThatThrownBy(() -> {
+RequireJavaVersion rule = new RequireJavaVersion();
+rule.setVersion(requiredVersion);
+
+EnforcerRuleHelper helper = EnforcerTestUtils.getHelper();
+rule.execute(helper);
+})
+.isInstanceOf(EnforcerRuleException.class)
+.hasMessage("The requested JDK version %s is invalid.", 
requiredVersion);
 }
 
 @Test
-@Disabled
-// TODO: Think about the intention of this test? What should it prove?
-void thisShouldNotCrash() throws EnforcerRuleException {
-RequireJavaVersion rule = new RequireJavaVersion();
-rule.setVersion(SystemUtils.JAVA_VERSION);
-
-EnforcerRuleHelper helper = EnforcerTestUtils.getHelper();
-rule.execute(helper);
-// intentionally no assertThat(...) because we don't expect and 
exception.

Review Comment:
   This test has been disabled for years now and as far as I can tell it is not 
really testing anything that is not being already tested elsewhere, so I 
figured we should just delete it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

2022-12-29 Thread GitBox



psiroky commented on code in PR #213:
URL: https://github.com/apache/maven-enforcer/pull/213#discussion_r1059280557


##
enforcer-rules/src/test/java/org/apache/maven/plugins/enforcer/TestRequireJavaVendor.java:
##
@@ -56,25 +54,33 @@ public void matchingInclude() throws EnforcerRuleException {
 }
 
 @Test
-public void nonMatchingInclude() {
+void nonMatchingInclude() {
 // Set the included vendor to something irrelevant
 underTest.setIncludes(Collections.singletonList(NON_MATCHING_VENDOR));
 final EnforcerRuleHelper helper = EnforcerTestUtils.getHelper();
-EnforcerRuleException e = assertThrows(EnforcerRuleException.class, () 
-> underTest.execute(helper));
-assertThat(e.getMessage(), is(SystemUtils.JAVA_VENDOR + " is not an 
included Required Java Vendor"));
+
+assertThatThrownBy(() -> underTest.execute(helper))
+.isInstanceOf(EnforcerRuleException.class)
+.hasMessage(
+"%s is not an included Required Java Vendor 
(JAVA_HOME=%s)",
+SystemUtils.JAVA_VENDOR, SystemUtils.JAVA_HOME);

Review Comment:
   I replaced the exception assserts with AssertJ since I find those much 
easier to read and understand (and I noticed the project uses asserj elsewhere 
as well).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages

6 matches

Site Navigation

Mail list logo

Footer information