[GitHub] [tomcat-jakartaee-migration] wmccusker commented on a diff in pull request #33: fixes #32 check if file name ends with manifest name
wmccusker commented on code in PR #33: URL: https://github.com/apache/tomcat-jakartaee-migration/pull/33#discussion_r955189029 ## src/test/java/org/apache/tomcat/jakartaee/ManifestConverterTest.java: ## @@ -0,0 +1,16 @@ +package org.apache.tomcat.jakartaee; + +import static org.junit.Assert.assertTrue; Review Comment: Yes, that is the general recommendation for why static imports are not used. In this specific case, it is a very common and accepted practice that the Assert class from Junit is imported using a static import. As mentioned before, the static import is consistent with all the existing Junit test code in this project. Are you suggesting that this single Junit test should break with the consistency of how all the other Junit tests are written? That only this unit test should do something different than all the other unit test code that is already in the repository? Using a non-static import, in this case, would be less intuitive because it would introduce two different conventions for how the Asset class is handled in unit tests. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] wmccusker commented on a diff in pull request #33: fixes #32 check if file name ends with manifest name
wmccusker commented on code in PR #33: URL: https://github.com/apache/tomcat-jakartaee-migration/pull/33#discussion_r951564802 ## src/test/java/org/apache/tomcat/jakartaee/ManifestConverterTest.java: ## @@ -0,0 +1,16 @@ +package org.apache.tomcat.jakartaee; + +import static org.junit.Assert.assertTrue; Review Comment: I reviewed all the unit test code in the jakartaee package, and it appears that all of them use static imports for Assert. Since this static import is consistent with all other JUnit tests in the package and since it is limited to only the Assert class, does this address this comment? -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] wmccusker commented on a diff in pull request #33: fixes #32 check if file name ends with manifest name
wmccusker commented on code in PR #33: URL: https://github.com/apache/tomcat-jakartaee-migration/pull/33#discussion_r951564802 ## src/test/java/org/apache/tomcat/jakartaee/ManifestConverterTest.java: ## @@ -0,0 +1,16 @@ +package org.apache.tomcat.jakartaee; + +import static org.junit.Assert.assertTrue; Review Comment: I reviewed all the unit test code in the jakartaee package, and it appears that all of them use static imports for Assert. Also it is fairly comment for static imports to be used on the Asset class of JUnit. Consider the documentation for Unit 4 and 5 both use a static import in their very first examples of writing tests. https://junit.org/junit5/docs/current/user-guide/#writing-tests https://github.com/junit-team/junit4/wiki/Getting-started#create-a-test While use of static imports in general should be discouraged, in this very narrow and specific case of the Assert class from the JUnit library it is a fairly common practice to use static imports. Since this static import is consistent with all other JUnit tests in the package and since it is limited to only the Assert class, does this address this comment? -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] wmccusker commented on a diff in pull request #33: fixes #32 check if file name ends with manifest name
wmccusker commented on code in PR #33: URL: https://github.com/apache/tomcat-jakartaee-migration/pull/33#discussion_r951564802 ## src/test/java/org/apache/tomcat/jakartaee/ManifestConverterTest.java: ## @@ -0,0 +1,16 @@ +package org.apache.tomcat.jakartaee; + +import static org.junit.Assert.assertTrue; Review Comment: I reviewed all the unit test code in the jakartaee package, and it appears that all of them use static imports for Assert. Does this address this comment? -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] wmccusker commented on a diff in pull request #33: fixes #32 check if file name ends with manifest name
wmccusker commented on code in PR #33:
URL:
https://github.com/apache/tomcat-jakartaee-migration/pull/33#discussion_r951565338
##
src/test/java/org/apache/tomcat/jakartaee/ManifestConverterTest.java:
##
@@ -0,0 +1,16 @@
+package org.apache.tomcat.jakartaee;
+
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+public class ManifestConverterTest {
+
+ @Test
+public void testAccepts() {
Review Comment:
I pushed an additional commit that fixes the whitespace formatting. Does it
address this comment?
--
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: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] wmccusker commented on a diff in pull request #33: fixes #32 check if file name ends with manifest name
wmccusker commented on code in PR #33: URL: https://github.com/apache/tomcat-jakartaee-migration/pull/33#discussion_r951564802 ## src/test/java/org/apache/tomcat/jakartaee/ManifestConverterTest.java: ## @@ -0,0 +1,16 @@ +package org.apache.tomcat.jakartaee; + +import static org.junit.Assert.assertTrue; Review Comment: I reviewed all the unit test code in the jakartaee package, and it appears that all of them use static imports for Assert. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
