Improve consistency of struts with existing tests (#1665)

* Improve consistency of struts with existing tests

* spotless

instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy

@@ -3,6 +3,9 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
+
 import io.opentelemetry.api.trace.Span
 import io.opentelemetry.instrumentation.test.asserts.TraceAssert
 import io.opentelemetry.instrumentation.test.base.HttpServerTest
@@ -14,10 +17,6 @@ import org.eclipse.jetty.servlet.DefaultServlet
 import org.eclipse.jetty.servlet.ServletContextHandler
 import org.eclipse.jetty.util.resource.FileResource
 
-import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
-import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
-import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
-
 class Struts2ActionSpanTest extends HttpServerTest<Server> {
 
   @Override
@@ -36,12 +35,7 @@ class Struts2ActionSpanTest extends HttpServerTest<Server> {
   }
 
   @Override
-  boolean testError() {
+  boolean testErrorBody() {
-    return false
-  }
-
-  @Override
-  boolean testRedirect() {
     return false
   }
 
@@ -54,29 +48,16 @@ class Struts2ActionSpanTest extends HttpServerTest<Server> {
     return endpoint == PATH_PARAM ? getContextPath() + "/path/{id}/param" : endpoint.resolvePath(address).path
   }
 
-  String expectedHandlerName(ServerEndpoint serverEndpoint) {
-    return "GreetingAction." + expectedMethodName(serverEndpoint)
-  }
-
-  String expectedMethodName(ServerEndpoint endpoint) {
-    switch (endpoint) {
-      case QUERY_PARAM: return "query"
-      case EXCEPTION: return "exception"
-      case PATH_PARAM: return "pathParam"
-      default: return "success"
-    }
-  }
-
   @Override
   void handlerSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) {
     trace.span(index) {
-      name expectedHandlerName(endpoint)
+      name "GreetingAction.${endpoint.name().toLowerCase()}"
       kind Span.Kind.INTERNAL
       errored endpoint == EXCEPTION
       if (endpoint == EXCEPTION) {
         errorEvent(Exception, EXCEPTION.body)
       }
-      def expectedMethodName = expectedMethodName(endpoint)
+      def expectedMethodName = endpoint.name().toLowerCase()
       attributes {
         'code.namespace' "io.opentelemetry.struts.GreetingAction"
         'code.function' expectedMethodName

instrumentation/struts-2.3/javaagent/src/test/java/io/opentelemetry/struts/GreetingAction.java

@@ -20,7 +20,15 @@ public class GreetingAction extends ActionSupport {
     return "greeting";
   }
 
-  public String query() {
+  public String redirect() {
+    responseBody =
+        HttpServerTest.controller(
+            HttpServerTest.ServerEndpoint.REDIRECT,
+            HttpServerTest.ServerEndpoint.REDIRECT::getBody);
+    return "redirect";
+  }
+
+  public String query_param() {
     responseBody =
         HttpServerTest.controller(
             HttpServerTest.ServerEndpoint.QUERY_PARAM,
@@ -28,17 +36,22 @@ public class GreetingAction extends ActionSupport {
     return "greeting";
   }
 
+  public String error() {
+    HttpServerTest.controller(
+        HttpServerTest.ServerEndpoint.ERROR, HttpServerTest.ServerEndpoint.ERROR::getBody);
+    return "error";
+  }
+
   public String exception() {
-    responseBody =
     HttpServerTest.controller(
         HttpServerTest.ServerEndpoint.EXCEPTION,
         () -> {
           throw new Exception(HttpServerTest.ServerEndpoint.EXCEPTION.getBody());
         });
-    return "exception";
+    throw new AssertionError(); // should not reach here
   }
 
-  public String pathParam() {
+  public String path_param() {
     HttpServerTest.controller(
         HttpServerTest.ServerEndpoint.PATH_PARAM,
         () ->
@@ -50,11 +63,6 @@ public class GreetingAction extends ActionSupport {
     responseBody = id;
   }
 
-  public void setSome(String some) {
-    responseBody = "some=" + some;
-    System.out.println("Setting query param some to " + some);
-  }
-
   public String getResponseBody() {
     return responseBody;
   }

instrumentation/struts-2.3/javaagent/src/test/resources/struts.xml

@@ -5,27 +5,32 @@
 
 <struts>
 
-  <constant name="struts.devMode" value="true" />
   <constant name="struts.enable.SlashesInActionNames" value="true"/>
-  <constant name="struts.mapper.alwaysSelectFullNamespace" value="false"/>
+  <constant name="struts.patternMatcher" value="namedVariable"/>
-  <constant name="struts.patternMatcher" value="namedVariable" />
 
   <package name="basic-struts2" extends="struts-default">
     <global-results>
-      <result name="exception" type="httpheader">
+      <result name="redirect" type="redirect">
+        <param name="location">/redirected</param>
+        <param name="prependServletContext">false</param>
+      </result>
+      <result name="error" type="httpheader">
         <param name="error">500</param>
       </result>
-      <result type="freemarker" name="greeting">greeting.ftl</result>
+      <result type="freemarker" name="greeting">/greeting.ftl</result>
     </global-results>
 
     <global-exception-mappings>
-      <exception-mapping exception="java.lang.Exception" result="exception" />
+      <exception-mapping exception="java.lang.Exception" result="error"/>
     </global-exception-mappings>
 
     <action name="success" class="io.opentelemetry.struts.GreetingAction" method="success"/>
-    <action name="query" class="io.opentelemetry.struts.GreetingAction" method="query"/>
+    <action name="redirect" class="io.opentelemetry.struts.GreetingAction" method="redirect"/>
+    <action name="query" class="io.opentelemetry.struts.GreetingAction" method="query_param"/>
+    <action name="error-status" class="io.opentelemetry.struts.GreetingAction" method="error"/>
     <action name="exception" class="io.opentelemetry.struts.GreetingAction" method="exception"/>
-    <action name="/path/{id}/param" class="io.opentelemetry.struts.GreetingAction" method="pathParam"/>
+    <action name="/path/{id}/param" class="io.opentelemetry.struts.GreetingAction"
+      method="path_param"/>
   </package>
 
 

instrumentation/struts-2.3/javaagent/struts-2.3-javaagent.gradle

@@ -4,12 +4,12 @@ muzzle {
   pass {
     group = "org.apache.struts"
     module = "struts2-core"
-    versions = "[2.3.20,)"
+    versions = "[2.3.1,)"
   }
 }
 
 dependencies {
-  library group: 'org.apache.struts', name: 'struts2-core', version: '2.3.20'
+  library group: 'org.apache.struts', name: 'struts2-core', version: '2.3.1'
 
   testImplementation(project(':testing-common')) {
     exclude group: 'org.eclipse.jetty', module: 'jetty-server'

testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy

@@ -16,13 +16,13 @@ import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTra
 import static org.junit.Assume.assumeTrue
 
 import ch.qos.logback.classic.Level
+import io.opentelemetry.api.trace.Span
+import io.opentelemetry.api.trace.attributes.SemanticAttributes
 import io.opentelemetry.instrumentation.test.AgentTestRunner
 import io.opentelemetry.instrumentation.test.asserts.TraceAssert
 import io.opentelemetry.instrumentation.test.utils.OkHttpUtils
 import io.opentelemetry.instrumentation.test.utils.PortUtils
 import io.opentelemetry.sdk.trace.data.SpanData
-import io.opentelemetry.api.trace.Span
-import io.opentelemetry.api.trace.attributes.SemanticAttributes
 import java.util.concurrent.Callable
 import okhttp3.HttpUrl
 import okhttp3.OkHttpClient
@@ -120,6 +120,10 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
     false
   }
 
+  boolean testErrorBody() {
+    return true
+  }
+
   boolean testExceptionBody() {
     true
   }
@@ -128,14 +132,6 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
     true
   }
 
-  boolean testRedirect() {
-    true
-  }
-
-  boolean testError() {
-    true
-  }
-
   enum ServerEndpoint {
     SUCCESS("success", 200, "success"),
     REDIRECT("redirect", 302, "/redirected"),
@@ -286,7 +282,6 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
 
   def "test redirect"() {
     setup:
-    assumeTrue(testRedirect())
     def request = request(REDIRECT, method, body).build()
     def response = client.newCall(request).execute()
 
@@ -306,13 +301,14 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
 
   def "test error"() {
     setup:
-    assumeTrue(testError())
     def request = request(ERROR, method, body).build()
     def response = client.newCall(request).execute()
 
     expect:
     response.code() == ERROR.status
+    if (testErrorBody()) {
       response.body().string() == ERROR.body
+    }
 
     and:
     assertTheTraces(1, null, null, method, ERROR, null, response)