csutherl commented on code in PR #1146:
URL: https://github.com/apache/tomcat/pull/1146#discussion_r4493352674
##########
test/org/apache/tomcat/categories/SmokeSuite.java:
##########
Review Comment:
When I suggested using Categories I'd assumed that they were discoverable
annotations that didn't need to have the classes also listed like this :( This
is problematic because leaving the test-profiles properties file and adding
this suite and the requirement to add SmokeTest annotations is duplicating a
lot and will definitely drift. Is there a way to utilize the annotation in a
way that makes it discoverable instead of using a Suite class for every
Category we decide to use?
##########
BUILDING.txt:
##########
@@ -603,7 +603,49 @@ NOTE: Cobertura is licensed under GPL v2 with parts of it
being under
test.verbose=false
-(7.5) Running httpd integration tests
+(7.5) Running tests by profile
+
+Tomcat provides named test profiles for running curated subsets of tests
+without specifying file patterns manually.
+
+Available profiles:
+
+ smoke - Core smoke tests covering key subsystems
+ catalina - All Catalina servlet container tests
+ coyote - All Coyote protocol/connector tests
+ tribes - All Tribes clustering tests
+ performance - Performance benchmark tests (excluded by default)
+ buildutil - Build utility tests (excluded by default)
+ integration - httpd integration tests (excluded by default)
+
+Usage:
+
+ ant test -Dtest.profile=smoke
+ ant test -Dtest.profile=catalina
+
+Profile patterns are defined in test-profiles.properties.default and can
+be overridden by creating a test-profiles.properties file at the project root.
+
+(7.5.1) Running smoke tests via JUnit @Category (proof of concept)
Review Comment:
We don't need to call out this as a proof of concept here.
##########
test-profiles.properties.default:
##########
@@ -43,9 +43,9 @@ org/apache/catalina/servlets/TestDefaultServlet.java,\
org/apache/catalina/servlets/TestDefaultServletOptions.java,\
org/apache/catalina/servlets/TestWebdavServletOptionsFile.java,\
org/apache/catalina/startup/TestContextConfig.java,\
-org/apache/catalina/startup/TestHostConfigAutomaticDeploymentA.java,\
-org/apache/catalina/startup/TestHostConfigAutomaticDeploymentB.java,\
-org/apache/catalina/startup/TestHostConfigAutomaticDeploymentC.java,\
+org/apache/catalina/startup/TestHostConfigAutomaticDeploymentDir.java,\
+org/apache/catalina/startup/TestHostConfigAutomaticDeploymentWar.java,\
+org/apache/catalina/startup/TestHostConfigAutomaticDeploymentXml.java,\
Review Comment:
This is a problem that should be addressed in an individual commit as the
classes were renamed and it wasn't caught before. I suggest splitting this fix
out into a separate commit.
##########
webapps/docs/changelog.xml:
##########
@@ -1708,6 +1694,14 @@
<update>
Update Tomcat Native to 2.0.17. (markt)
</update>
+ <add>
+ Introduce JUnit <code>@Category</code>-based smoke test suite as a
+ proof of concept. Annotate smoke tests with
Review Comment:
It's odd to put that it's a POC in the changelog. It's just the first change
in a set of changes we'd add to make it work. If it get's removed later we can
just add another entry saying so.
##########
build.xml:
##########
Review Comment:
The new test-smoke-category target is problematic because it duplicates a
bunch of the config from the runtests macro and will likely become stale
overtime. Can we not implement the target without the duplication?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
