Merge pull request #383 from cqse/ts/36706_fix_maven_cucumber

TS-36706 Escape Scenario Names in Cucumber
cqse · Dec 14, 2023 · 1960aa4 · 1960aa4
2 parents 9a6846f + 5d98470
commit 1960aa4
Show file tree

Hide file tree

Showing 16 changed files with 105 additions and 39 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,8 @@ We use [semantic versioning](http://semver.org/):
 - PATCH version when you make backwards compatible bug fixes.
 
 # Next Release
+- [fix] _teamscale-maven-plugin_ Test names containing slashes could not be uploaded
+- [fix] _tia-client_ Impacted test retrieval failed due to JSON parsing error
 
 # 32.6.0
 - [feature] Profiler logs its version on startup

diff --git a/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/CoverageToJsonStrategyBase.java b/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/CoverageToJsonStrategyBase.java
@@ -1,6 +1,7 @@
 package com.teamscale.jacoco.agent.testimpact;
 
 import com.teamscale.client.ClusteredTestDetails;
+import com.teamscale.client.JsonUtils;
 import com.teamscale.client.PrioritizableTestCluster;
 import com.teamscale.jacoco.agent.JacocoRuntimeController;
 import com.teamscale.jacoco.agent.options.AgentOptions;
@@ -13,7 +14,6 @@
 import com.teamscale.report.testwise.model.TestwiseCoverageReport;
 import com.teamscale.report.testwise.model.builder.TestCoverageBuilder;
 import com.teamscale.report.testwise.model.builder.TestwiseCoverageReportBuilder;
-import com.teamscale.report.util.JsonUtils;
 import org.slf4j.Logger;
 
 import java.io.File;

diff --git a/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/TestExecutionWriter.java b/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/TestExecutionWriter.java
@@ -1,8 +1,8 @@
 package com.teamscale.jacoco.agent.testimpact;
 
+import com.teamscale.client.JsonUtils;
 import com.teamscale.jacoco.agent.util.LoggingUtils;
 import com.teamscale.report.testwise.model.TestExecution;
-import com.teamscale.report.util.JsonUtils;
 import org.slf4j.Logger;
 
 import java.io.File;

diff --git a/common-system-test/src/main/java/com/teamscale/test/commons/TeamscaleMockServer.java b/common-system-test/src/main/java/com/teamscale/test/commons/TeamscaleMockServer.java
@@ -1,10 +1,10 @@
 package com.teamscale.test.commons;
 
+import com.teamscale.client.JsonUtils;
 import com.teamscale.client.PrioritizableTest;
 import com.teamscale.client.PrioritizableTestCluster;
 import com.teamscale.client.TestWithClusterId;
 import com.teamscale.report.testwise.model.TestwiseCoverageReport;
-import com.teamscale.report.util.JsonUtils;
 import spark.Request;
 import spark.Response;
 import spark.Service;

diff --git a/...in/java/com/teamscale/test_impacted/test_descriptor/CucumberPickleDescriptorResolver.java b/...in/java/com/teamscale/test_impacted/test_descriptor/CucumberPickleDescriptorResolver.java
@@ -115,12 +115,28 @@ private Optional<String> getPickleName(TestDescriptor testDescriptor) {
 			Method getNameMethod = pickle.getClass().getDeclaredMethod("getName");
 			getNameMethod.setAccessible(true);
 			String name = getNameMethod.invoke(pickle).toString();
-			return Optional.of(name);
+
+			return Optional
+					.of(name)
+					.map(CucumberPickleDescriptorResolver::escapeSlashes);
 		} catch (NoSuchFieldException | InvocationTargetException | IllegalAccessException | NoSuchMethodException e) {
 			return Optional.empty();
 		}
 	}
 
+	/**
+	 * Escapes slashes (/) in a given input (usually a scenario name) with a backslash (\).
+	 * <br><br>
+	 * If a slash is already escaped, no additional escaping is done.
+	 * <ul>
+	 *     <li>{@code / -> \/}</li>
+	 *     <li>{@code \/ -> \/}</li>
+	 * </ul>
+	 */
+	static String escapeSlashes(String input) {
+		return input.replaceAll("(?<!\\\\)/", "\\\\/");
+	}
+
 	private Optional<TestDescriptor> getFeatureFileTestDescriptor(TestDescriptor testDescriptor) {
 		if (!isFeatureFileTestDescriptor(testDescriptor)) {
 			if (!testDescriptor.getParent().isPresent()) {

diff --git a/...ava/com/teamscale/test_impacted/test_descriptor/CucumberPickleDescriptorResolverTest.java b/...ava/com/teamscale/test_impacted/test_descriptor/CucumberPickleDescriptorResolverTest.java
@@ -0,0 +1,30 @@
+package com.teamscale.test_impacted.test_descriptor;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.util.LinkedHashMap;
+
+class CucumberPickleDescriptorResolverTest {
+
+	@Test
+	void escapeSlashes() {
+		LinkedHashMap<String, String> expectedByInput = new LinkedHashMap<>();
+		expectedByInput.put("abc", "abc");
+		expectedByInput.put("ab/c", "ab\\/c");
+		expectedByInput.put("ab\\/c", "ab\\/c"); // don't escape what is already escaped
+		expectedByInput.put("/abc", "\\/abc");
+		expectedByInput.put("/abc/", "\\/abc\\/");
+		expectedByInput.put("/", "\\/");
+		expectedByInput.put("/a/", "\\/a\\/");
+		expectedByInput.put("a//", "a\\/\\/");
+		expectedByInput.put("//a", "\\/\\/a");
+		expectedByInput.put("//", "\\/\\/");
+		expectedByInput.put("///", "\\/\\/\\/");
+		expectedByInput.put("\\", "\\");
+		expectedByInput.put("http://link", "http:\\/\\/link");
+
+		expectedByInput.forEach((input, expected) -> Assertions.assertEquals(expected,
+				CucumberPickleDescriptorResolver.escapeSlashes(input)));
+	}
+}
diff --git a/report-generator/src/main/java/com/teamscale/report/ReportUtils.java b/report-generator/src/main/java/com/teamscale/report/ReportUtils.java
@@ -2,11 +2,11 @@
 
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.teamscale.client.FileSystemUtils;
+import com.teamscale.client.JsonUtils;
 import com.teamscale.client.TestDetails;
 import com.teamscale.report.testwise.ETestArtifactFormat;
 import com.teamscale.report.testwise.model.TestExecution;
 import com.teamscale.report.testwise.model.TestwiseCoverageReport;
-import com.teamscale.report.util.JsonUtils;
 
 import java.io.File;
 import java.io.IOException;

diff --git a/...t-generator/src/main/java/com/teamscale/report/testwise/TestwiseCoverageReportWriter.java b/...t-generator/src/main/java/com/teamscale/report/testwise/TestwiseCoverageReportWriter.java
@@ -2,11 +2,11 @@
 
 import com.fasterxml.jackson.core.JsonGenerator;
 import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
+import com.teamscale.client.JsonUtils;
 import com.teamscale.client.StringUtils;
 import com.teamscale.report.testwise.model.TestInfo;
 import com.teamscale.report.testwise.model.builder.TestCoverageBuilder;
 import com.teamscale.report.testwise.model.factory.TestInfoFactory;
-import com.teamscale.report.util.JsonUtils;
 
 import java.io.File;
 import java.io.IOException;

diff --git a/...ests/cucumber-maven-tia/maven-project/src/test/resources/hellocucumber/calculator.feature b/...ests/cucumber-maven-tia/maven-project/src/test/resources/hellocucumber/calculator.feature
@@ -34,3 +34,8 @@ Feature: Calculator
       | num1 | num2 | difference |
       | 99   | 99   | 0          |
       | 10   | -1   | 11         |
+
+  Scenario: Actually we just want to test a http://link
+    Given I have a calculator
+    When I add 0 and 1
+    Then the result should be 1
diff --git a/...-tests/cucumber-maven-tia/src/test/java/com/teamscale/tia/TiaMavenCucumberSystemTest.java b/...-tests/cucumber-maven-tia/src/test/java/com/teamscale/tia/TiaMavenCucumberSystemTest.java
@@ -8,6 +8,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import java.util.Arrays;
+
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertAll;
 
@@ -24,16 +26,24 @@ public class TiaMavenCucumberSystemTest {
 	private static final int FAKE_TEAMSCALE_PORT = 63800;
 	private static TeamscaleMockServer teamscaleMockServer = null;
 
+	private static final String[] IMPACTED_TEST_PATHS = { // sorted alphabetically
+			"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Actually we just want to test a http:\\/\\/link #1", // also tests addition, escaped /
+			"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #1",
+			"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #2",
+			"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #3",
+			"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #4",
+			"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Subtract two numbers 99 & 99 #1"
+	};
+
+	private static final String COVERAGE_ADD = "Calculator.java:3,5;StepDefinitions.java:12,24-25,29-30,39-40";
+	private static final String COVERAGE_SUBTRACT = "Calculator.java:3,9;StepDefinitions.java:12,24-25,34-35,39-40";
+
 	@BeforeEach
 	public void startFakeTeamscaleServer() throws Exception {
 		if (teamscaleMockServer == null) {
-			teamscaleMockServer = new TeamscaleMockServer(FAKE_TEAMSCALE_PORT).acceptingReportUploads()
-					.withImpactedTests(
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #1",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #2",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #3",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #4",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Subtract two numbers 99 & 99 #1");
+			teamscaleMockServer = new TeamscaleMockServer(FAKE_TEAMSCALE_PORT)
+					.acceptingReportUploads()
+					.withImpactedTests(IMPACTED_TEST_PATHS);
 		}
 		teamscaleMockServer.uploadedReports.clear();
 	}
@@ -52,26 +62,25 @@ public void testMavenTia() throws Exception {
 		assertThat(teamscaleMockServer.uploadedReports).hasSize(1);
 
 		TestwiseCoverageReport unitTestReport = teamscaleMockServer.parseUploadedTestwiseCoverageReport(0);
-		assertThat(unitTestReport.tests).hasSize(5);
+		assertThat(unitTestReport.tests).hasSize(IMPACTED_TEST_PATHS.length);
 		assertThat(unitTestReport.partial).isTrue();
 		assertAll(() -> {
-			assertThat(unitTestReport.tests).extracting(test -> test.uniformPath)
-					.containsExactlyInAnyOrder(
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #1",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #2",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #3",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Add two numbers #4",
-							"hellocucumber/RunCucumberTest/hellocucumber/calculator.feature/Subtract two numbers 99 & 99 #1");
-			assertThat(unitTestReport.tests).extracting(test -> test.result)
-					.containsExactlyInAnyOrder(ETestExecutionResult.PASSED, ETestExecutionResult.PASSED,
-							ETestExecutionResult.PASSED, ETestExecutionResult.PASSED, ETestExecutionResult.PASSED);
-			assertThat(unitTestReport.tests).extracting(SystemTestUtils::getCoverageString)
-					.containsExactly(
-							"Calculator.java:3,5;StepDefinitions.java:12,24-25,29-30,39-40",
-							"Calculator.java:3,5;StepDefinitions.java:12,24-25,29-30,39-40",
-							"Calculator.java:3,5;StepDefinitions.java:12,24-25,29-30,39-40",
-							"Calculator.java:3,5;StepDefinitions.java:12,24-25,29-30,39-40",
-							"Calculator.java:3,9;StepDefinitions.java:12,24-25,34-35,39-40");
+			assertThat(unitTestReport.tests)
+					.extracting(test -> test.uniformPath)
+					.containsExactlyInAnyOrder(IMPACTED_TEST_PATHS);
+			assertThat(unitTestReport.tests)
+					.extracting(test -> test.result)
+					.containsExactlyInAnyOrder(Arrays.stream(IMPACTED_TEST_PATHS)
+							.map(s -> ETestExecutionResult.PASSED)
+							.toArray(ETestExecutionResult[]::new));
+			assertThat(unitTestReport.tests)
+					.extracting(SystemTestUtils::getCoverageString)
+					.containsExactly(COVERAGE_ADD, // must match TEST_PATHS
+							COVERAGE_ADD,
+							COVERAGE_ADD,
+							COVERAGE_ADD,
+							COVERAGE_ADD,
+							COVERAGE_SUBTRACT);
 		});
 	}
 

diff --git a/system-tests/tia-maven/src/test/java/com/teamscale/tia/TiaMavenDumpToFileSystemTest.java b/system-tests/tia-maven/src/test/java/com/teamscale/tia/TiaMavenDumpToFileSystemTest.java
@@ -1,8 +1,8 @@
 package com.teamscale.tia;
 
+import com.teamscale.client.JsonUtils;
 import com.teamscale.report.testwise.model.ETestExecutionResult;
 import com.teamscale.report.testwise.model.TestwiseCoverageReport;
-import com.teamscale.report.util.JsonUtils;
 import com.teamscale.test.commons.SystemTestUtils;
 import com.teamscale.test.commons.TeamscaleMockServer;
 import org.junit.jupiter.api.AfterEach;

diff --git a/.../com/teamscale/report/util/JsonUtils.java → .../java/com/teamscale/client/JsonUtils.java b/.../com/teamscale/report/util/JsonUtils.java → .../java/com/teamscale/client/JsonUtils.java
@@ -1,10 +1,11 @@
-package com.teamscale.report.util;
+package com.teamscale.client;
 
 import com.fasterxml.jackson.annotation.JsonAutoDetect;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.PropertyAccessor;
 import com.fasterxml.jackson.core.JsonFactory;
 import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.json.JsonMapper;
 
@@ -22,9 +23,10 @@ public class JsonUtils {
 	 * OBJECT_MAPPER are configured to include all fields when serializing or deserializing objects, regardless of their
 	 * visibility modifiers (public, private, etc.).
 	 */
-	private static final ObjectMapper OBJECT_MAPPER = JsonMapper.builder()
+	public static final ObjectMapper OBJECT_MAPPER = JsonMapper.builder()
 			.visibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE)
 			.visibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY)
+			.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
 			.serializationInclusion(JsonInclude.Include.NON_NULL)
 			.build();
 

diff --git a/teamscale-client/src/main/java/com/teamscale/client/TeamscaleServiceGenerator.java b/teamscale-client/src/main/java/com/teamscale/client/TeamscaleServiceGenerator.java
@@ -24,7 +24,8 @@ public class TeamscaleServiceGenerator {
 	public static <S> S createService(Class<S> serviceClass, HttpUrl baseUrl, String username, String accessToken,
 									  int readTimeout, int writeTimeout, Interceptor... interceptors) {
 		Retrofit retrofit = HttpUtils.createRetrofit(
-				retrofitBuilder -> retrofitBuilder.baseUrl(baseUrl).addConverterFactory(JacksonConverterFactory.create()),
+				retrofitBuilder -> retrofitBuilder.baseUrl(baseUrl)
+						.addConverterFactory(JacksonConverterFactory.create(JsonUtils.OBJECT_MAPPER)),
 				okHttpBuilder -> addInterceptors(okHttpBuilder, interceptors)
 						.addInterceptor(HttpUtils.getBasicAuthInterceptor(username, accessToken))
 						.addInterceptor(new AcceptJsonInterceptor())
@@ -42,7 +43,8 @@ public static <S> S createServiceWithRequestLogging(Class<S> serviceClass, HttpU
 														String accessToken, File logfile, int readTimeout,
 														int writeTimeout, Interceptor... interceptors) {
 		Retrofit retrofit = HttpUtils.createRetrofit(
-				retrofitBuilder -> retrofitBuilder.baseUrl(baseUrl).addConverterFactory(JacksonConverterFactory.create()),
+				retrofitBuilder -> retrofitBuilder.baseUrl(baseUrl)
+						.addConverterFactory(JacksonConverterFactory.create(JsonUtils.OBJECT_MAPPER)),
 				okHttpBuilder -> addInterceptors(okHttpBuilder, interceptors)
 						.addInterceptor(HttpUtils.getBasicAuthInterceptor(username, accessToken))
 						.addInterceptor(new AcceptJsonInterceptor())

diff --git a/teamscale-gradle-plugin/src/test/kotlin/com/teamscale/TeamscalePluginTest.kt b/teamscale-gradle-plugin/src/test/kotlin/com/teamscale/TeamscalePluginTest.kt
@@ -1,9 +1,9 @@
 package com.teamscale
 
 import com.teamscale.TestwiseCoverageReportAssert.Companion.assertThat
+import com.teamscale.client.JsonUtils
 import com.teamscale.report.testwise.model.ETestExecutionResult
 import com.teamscale.report.testwise.model.TestwiseCoverageReport
-import com.teamscale.report.util.JsonUtils
 import com.teamscale.test.commons.TeamscaleMockServer
 import org.assertj.core.api.Assertions.assertThat
 import org.gradle.testkit.runner.BuildResult

diff --git a/tia-client/src/main/java/com/teamscale/tia.client/CommandLineInterface.java b/tia-client/src/main/java/com/teamscale/tia.client/CommandLineInterface.java
@@ -1,11 +1,11 @@
 package com.teamscale.tia.client;
 
 import com.teamscale.client.ClusteredTestDetails;
+import com.teamscale.client.JsonUtils;
 import com.teamscale.client.PrioritizableTestCluster;
 import com.teamscale.client.StringUtils;
 import com.teamscale.report.testwise.model.ETestExecutionResult;
 import com.teamscale.report.testwise.model.TestExecution;
-import com.teamscale.report.util.JsonUtils;
 import okhttp3.HttpUrl;
 
 import java.io.BufferedReader;

diff --git a/tia-client/src/main/java/com/teamscale/tia.client/RunningTest.java b/tia-client/src/main/java/com/teamscale/tia.client/RunningTest.java
@@ -1,9 +1,9 @@
 package com.teamscale.tia.client;
 
+import com.teamscale.client.JsonUtils;
 import com.teamscale.client.StringUtils;
 import com.teamscale.report.testwise.model.TestExecution;
 import com.teamscale.report.testwise.model.TestInfo;
-import com.teamscale.report.util.JsonUtils;
 import okhttp3.ResponseBody;
 
 import java.io.IOException;