[GitHub] [maven-enforcer] psiroky commented on a diff in pull request #213: [MENFORCER-445] Include JAVA_HOME location in the Java rule failure messages
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
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
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
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
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
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