Merge pull request #711 from cqse/ts/42917_service_npe

DreierF · web-flow · commit afdd6076d210 · 2025-06-05T13:45:59.000+02:00
TS-42917 Ensure partition and clusterId are not null
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@ We use [semantic versioning](http://semver.org/):
 - PATCH version when you make backwards compatible bug fixes.
 
 # Next version
+- [fix] _agent_: Bad request error when starting a test run in some cases
 
 # 35.2.0
 - [feature] _agent_: Access token can be passed via environment variable `TEAMSCALE_ACCESS_TOKEN`
diff --git a/agent/src/main/java/com/teamscale/jacoco/agent/ResourceBase.java b/agent/src/main/java/com/teamscale/jacoco/agent/ResourceBase.java
@@ -6,6 +6,7 @@
 import com.teamscale.jacoco.agent.logging.LoggingUtils;
 import com.teamscale.report.testwise.model.RevisionInfo;
 import org.conqat.lib.commons.string.StringUtils;
+import org.jetbrains.annotations.Contract;
 import org.slf4j.Logger;
 
 import javax.ws.rs.BadRequestException;
@@ -135,6 +136,7 @@ private RevisionInfo getRevisionInfo() {
 	/**
 	 * Handles bad requests to the endpoints.
 	 */
+	@Contract(value = "_ -> fail")
 	protected void handleBadRequest(String message) throws BadRequestException {
 		logger.error(message);
 		throw new BadRequestException(message);
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,20 +1,11 @@
 package com.teamscale.jacoco.agent.testimpact;
 
-import static java.util.stream.Collectors.toList;
-
-import java.io.File;
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
-
-import org.slf4j.Logger;
-
 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;
 import com.teamscale.jacoco.agent.logging.LoggingUtils;
+import com.teamscale.jacoco.agent.options.AgentOptions;
 import com.teamscale.report.testwise.jacoco.JaCoCoTestwiseReportGenerator;
 import com.teamscale.report.testwise.jacoco.cache.CoverageGenerationException;
 import com.teamscale.report.testwise.model.TestExecution;
@@ -23,6 +14,14 @@
 import com.teamscale.report.testwise.model.TestwiseCoverageReport;
 import com.teamscale.report.testwise.model.builder.TestCoverageBuilder;
 import com.teamscale.report.testwise.model.builder.TestwiseCoverageReportBuilder;
+import org.slf4j.Logger;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.util.stream.Collectors.toList;
 
 /**
  * Base for strategies that produce testwise coverage information in JSON and store or send this data further.
@@ -45,16 +44,16 @@ public abstract class CoverageToJsonStrategyBase extends TestEventHandlerStrateg
 	private final JaCoCoTestwiseReportGenerator reportGenerator;
 
 	public CoverageToJsonStrategyBase(JacocoRuntimeController controller, AgentOptions agentOptions,
-									  JaCoCoTestwiseReportGenerator reportGenerator) {
+			JaCoCoTestwiseReportGenerator reportGenerator) {
 		super(agentOptions, controller);
 		this.reportGenerator = reportGenerator;
 	}
 
 	@Override
 	public List<PrioritizableTestCluster> testRunStart(List<ClusteredTestDetails> availableTests,
-													   boolean includeNonImpactedTests,
-													   boolean includeAddedTests, boolean includeFailedAndSkipped,
-													   String baseline, String baselineRevision) throws IOException {
+			boolean includeNonImpactedTests,
+			boolean includeAddedTests, boolean includeFailedAndSkipped,
+			String baseline, String baselineRevision) throws IOException {
 		if (availableTests != null) {
 			this.availableTests = new ArrayList<>(availableTests);
 		}
@@ -65,17 +64,16 @@ public List<PrioritizableTestCluster> testRunStart(List<ClusteredTestDetails> av
 	@Override
 	public void testStart(String uniformPath) {
 		super.testStart(uniformPath);
-
 		if (availableTests.stream().noneMatch(test -> test.uniformPath.equals(uniformPath))) {
 			// ensure that we can at least generate a report for the tests that were actually run,
 			// even if the caller did not provide a list of tests up-front in testRunStart
-			availableTests.add(new ClusteredTestDetails(uniformPath, uniformPath, null, null, null));
+			availableTests.add(new ClusteredTestDetails(uniformPath, uniformPath, null, null));
 		}
 	}
 
 	@Override
 	public TestInfo testEnd(String test,
-							TestExecution testExecution) throws JacocoRuntimeController.DumpException, CoverageGenerationException {
+			TestExecution testExecution) throws JacocoRuntimeController.DumpException, CoverageGenerationException {
 		super.testEnd(test, testExecution);
 		if (testExecution != null) {
 			testExecutions.add(testExecution);
@@ -100,6 +98,7 @@ public TestInfo testEnd(String test,
 	public void testRunEnd(boolean partial) throws IOException, CoverageGenerationException {
 		if (testExecFile == null) {
 			logger.warn("Tried to end a test run that contained no tests!");
+			clearTestRun();
 			return;
 		}
 
@@ -132,16 +131,22 @@ private String createTestwiseCoverageReport(boolean partial) throws IOException,
 		reportGenerator.updateClassDirCache();
 		TestwiseCoverage testwiseCoverage = reportGenerator.convert(testExecFile);
 		logger.debug("Created testwise coverage report (containing coverage for tests `{}`)",
-				testwiseCoverage.getTests().values().stream().map(TestCoverageBuilder::getUniformPath).collect(toList()));
+				testwiseCoverage.getTests().values().stream().map(TestCoverageBuilder::getUniformPath)
+						.collect(toList()));
 
-		TestwiseCoverageReport report = TestwiseCoverageReportBuilder.createFrom(availableTests, testwiseCoverage.getTests().values(), testExecutions, partial);
+		TestwiseCoverageReport report = TestwiseCoverageReportBuilder.createFrom(availableTests,
+				testwiseCoverage.getTests().values(), testExecutions, partial);
 
 		testExecFile.delete();
 		testExecFile = null;
-		availableTests.clear();
-		testExecutions.clear();
+		clearTestRun();
 
 		return JsonUtils.serializeToJson(report);
 	}
 
+	private void clearTestRun() {
+		availableTests.clear();
+		testExecutions.clear();
+	}
+
 }
diff --git a/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/TestEventHandlerStrategyBase.java b/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/TestEventHandlerStrategyBase.java
@@ -4,6 +4,7 @@
 import com.teamscale.client.HttpUtils;
 import com.teamscale.client.PrioritizableTestCluster;
 import com.teamscale.client.TeamscaleClient;
+import com.teamscale.client.TestWithClusterId;
 import com.teamscale.jacoco.agent.JacocoRuntimeController;
 import com.teamscale.jacoco.agent.logging.LoggingUtils;
 import com.teamscale.jacoco.agent.options.AgentOptions;
@@ -17,6 +18,7 @@
 import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
+import java.util.stream.Collectors;
 
 /** Base class for strategies to handle test events. */
 public abstract class TestEventHandlerStrategyBase {
@@ -88,15 +90,20 @@ public List<PrioritizableTestCluster> testRunStart(List<ClusteredTestDetails> av
 			boolean includeAddedTests, boolean includeFailedAndSkipped,
 			String baseline, String baselineRevision) throws IOException {
 		int availableTestCount = 0;
+		List<TestWithClusterId> availableTestsWithClusterId = null;
 		if (availableTests != null) {
 			availableTestCount = availableTests.size();
+			availableTestsWithClusterId = availableTests.stream()
+					.map(availableTest -> TestWithClusterId.Companion.fromClusteredTestDetails(availableTest, getPartition()))
+					.collect(
+							Collectors.toList());
 		}
 		logger.debug("Test run started with {} available tests. baseline = {}, includeNonImpactedTests = {}",
 				availableTestCount, baseline, includeNonImpactedTests);
 		validateConfiguration();
 
 		Response<List<PrioritizableTestCluster>> response = teamscaleClient
-				.getImpactedTests(availableTests, baseline, baselineRevision,
+				.getImpactedTests(availableTestsWithClusterId, baseline, baselineRevision,
 						agentOptions.getTeamscaleServerOptions().commit,
 						agentOptions.getTeamscaleServerOptions().revision,
 						agentOptions.getTeamscaleServerOptions().repository,
@@ -114,6 +121,18 @@ public List<PrioritizableTestCluster> testRunStart(List<ClusteredTestDetails> av
 		}
 	}
 
+	/**
+	 * Returns the partition defined in the agent options. Asserts that the partition is defined.
+	 */
+	private String getPartition() {
+		String partition = agentOptions.getTeamscaleServerOptions().partition;
+		if (partition == null) {
+			throw new UnsupportedOperationException(
+					"You must provide a partition via the agent's '" + TeamscaleConfig.TEAMSCALE_PARTITION_OPTION + "' option or using the /partition REST endpoint.");
+		}
+		return partition;
+	}
+
 	private void validateConfiguration() {
 		if (teamscaleClient == null) {
 			throw new UnsupportedOperationException("You did not configure a connection to Teamscale in the agent." +
diff --git a/agent/src/main/java/com/teamscale/jacoco/agent/upload/teamscale/TeamscaleConfig.java b/agent/src/main/java/com/teamscale/jacoco/agent/upload/teamscale/TeamscaleConfig.java
@@ -32,6 +32,9 @@ public class TeamscaleConfig {
 	/** Option name that allows to specify a jar file that contains the branch name and timestamp in a MANIFEST.MF file. */
 	public static final String TEAMSCALE_COMMIT_MANIFEST_JAR_OPTION = "teamscale-commit-manifest-jar";
 
+	/** Option name that allows to specify a partition to which coverage should be uploaded to. */
+	public static final String TEAMSCALE_PARTITION_OPTION = "teamscale-partition";
+
 	private final ILogger logger;
 	private final FilePatternResolver filePatternResolver;
 
@@ -61,7 +64,7 @@ public boolean handleTeamscaleOptions(TeamscaleServer teamscaleServer,
 			case "teamscale-access-token":
 				teamscaleServer.userAccessToken = value;
 				return true;
-			case "teamscale-partition":
+			case TEAMSCALE_PARTITION_OPTION:
 				teamscaleServer.partition = value;
 				return true;
 			case TEAMSCALE_COMMIT_OPTION:
diff --git a/agent/src/test/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategyTest.java b/agent/src/test/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategyTest.java
@@ -90,7 +90,7 @@ public void testValidCallSequence(boolean useRevision) throws Exception {
 
 		strategy.testRunStart(
 				Collections.singletonList(
-						new ClusteredTestDetails("mytest", "mytest", "content", "cluster", "partition")), false,
+						new ClusteredTestDetails("mytest", "mytest", "content", "cluster")), false,
 				true, true,
 				null, null);
 		strategy.testStart("mytest");
diff --git a/agent/src/test/java/com/teamscale/jacoco/agent/testimpact/TestwiseCoverageAgentTest.java b/agent/src/test/java/com/teamscale/jacoco/agent/testimpact/TestwiseCoverageAgentTest.java
@@ -64,8 +64,8 @@ public class TestwiseCoverageAgentTest {
 	@Test
 	public void testAccessViaTiaClientAndReportUploadToTeamscale() throws Exception {
 		List<ClusteredTestDetails> availableTests = Arrays
-				.asList(new ClusteredTestDetails("test1", "test1", "content", "cluster", "partition"),
-						new ClusteredTestDetails("test2", "test2", "content", "cluster", "partition"));
+				.asList(new ClusteredTestDetails("test1", "test1", "content", "cluster"),
+						new ClusteredTestDetails("test2", "test2", "content", "cluster"));
 		List<PrioritizableTestCluster> impactedClusters = Collections
 				.singletonList(new PrioritizableTestCluster("cluster",
 						Collections.singletonList(new PrioritizableTest("test2"))));
diff --git a/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/engine/InternalImpactedTestEngine.kt b/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/engine/InternalImpactedTestEngine.kt
@@ -66,7 +66,7 @@ internal class InternalImpactedTestEngine(
 	 */
 	fun execute(request: ExecutionRequest) {
 		val rootTestDescriptor = request.rootTestDescriptor
-		val availableTests = getAvailableTests(rootTestDescriptor, partition)
+		val availableTests = getAvailableTests(rootTestDescriptor)
 
 		LOG.fine {
 			"Starting selection and sorting ${ImpactedTestEngine.ENGINE_ID}:\n${
diff --git a/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/engine/executor/ImpactedTestsProvider.kt b/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/engine/executor/ImpactedTestsProvider.kt
@@ -1,9 +1,9 @@
 package com.teamscale.test_impacted.engine.executor
 
-import com.teamscale.client.ClusteredTestDetails
 import com.teamscale.client.CommitDescriptor
 import com.teamscale.client.PrioritizableTestCluster
 import com.teamscale.client.TeamscaleClient
+import com.teamscale.client.TestWithClusterId
 import com.teamscale.test_impacted.commons.LoggerUtils.createLogger
 import retrofit2.Response
 import java.io.IOException
@@ -37,7 +37,7 @@ open class ImpactedTestsProvider(
 ) {
 	/** Queries Teamscale for impacted tests.  */
 	fun getImpactedTestsFromTeamscale(
-		availableTestDetails: List<ClusteredTestDetails>
+		availableTestDetails: List<TestWithClusterId>
 	): List<PrioritizableTestCluster>? {
 		try {
 			LOG.info { "Getting impacted tests..." }
@@ -73,7 +73,7 @@ open class ImpactedTestsProvider(
 	 */
 	private fun testCountIsPlausible(
 		testClusters: List<PrioritizableTestCluster>,
-		availableTestDetails: List<ClusteredTestDetails>
+		availableTestDetails: List<TestWithClusterId>
 	): Boolean {
 		val returnedTests = testClusters.stream().mapToLong {
 			it.tests?.size?.toLong() ?: 0
diff --git a/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/engine/executor/ImpactedTestsSorter.kt b/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/engine/executor/ImpactedTestsSorter.kt
@@ -1,20 +1,23 @@
 package com.teamscale.test_impacted.engine.executor
 
+import com.teamscale.client.TestWithClusterId.Companion.fromClusteredTestDetails
 import com.teamscale.test_impacted.engine.ImpactedTestEngine
 import com.teamscale.test_impacted.test_descriptor.TestDescriptorUtils.getAvailableTests
 import org.junit.platform.engine.TestDescriptor
 import java.util.*
 
 /**
-* Test sorter that requests impacted tests from Teamscale and rewrites the [TestDescriptor] to take the returned
-* order into account when executing the tests.
-*/
+ * Test sorter that requests impacted tests from Teamscale and rewrites the [TestDescriptor] to take the returned
+ * order into account when executing the tests.
+ */
 class ImpactedTestsSorter(private val impactedTestsProvider: ImpactedTestsProvider) : ITestSorter {
 
 	override fun selectAndSort(testDescriptor: TestDescriptor) {
-		val availableTests = getAvailableTests(testDescriptor, impactedTestsProvider.partition)
+		val availableTests = getAvailableTests(testDescriptor)
 
-		val testClusters = impactedTestsProvider.getImpactedTestsFromTeamscale(availableTests.testList)
+		val testClusters = impactedTestsProvider.getImpactedTestsFromTeamscale(
+			availableTests.testList
+				.map { fromClusteredTestDetails(it, impactedTestsProvider.partition) })
 
 		if (testClusters == null) {
 			ImpactedTestEngine.LOG.fine { "Falling back to execute all!" }
diff --git a/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/test_descriptor/TestDescriptorUtils.kt b/impacted-test-engine/src/main/kotlin/com/teamscale/test_impacted/test_descriptor/TestDescriptorUtils.kt
@@ -94,8 +94,7 @@ object TestDescriptorUtils {
 
 	/** Returns the [AvailableTests] contained within the root [TestDescriptor].  */
 	fun getAvailableTests(
-		rootTestDescriptor: TestDescriptor,
-		partition: String?
+		rootTestDescriptor: TestDescriptor
 	): AvailableTests {
 		val availableTests = AvailableTests()
 
@@ -125,8 +124,7 @@ object TestDescriptorUtils {
 					uniformPath.get(),
 					testDescriptor.source(),
 					null,
-					clusterId.get(),
-					partition
+					clusterId.get()
 				)
 				availableTests.add(testDescriptor.uniqueId, testDetails)
 			}
diff --git a/system-tests/tia-client-test/src/main/kotlin/testframework/CustomTestFramework.kt b/system-tests/tia-client-test/src/main/kotlin/testframework/CustomTestFramework.kt
@@ -32,7 +32,7 @@ class CustomTestFramework(private val agentPort: Int) {
 		val agent = TiaAgent(false, "http://localhost:$agentPort".toHttpUrl())
 		val testRun = agent.startTestRun(
 			allTests.keys
-				.map { name -> ClusteredTestDetails(name, name, null, null, null) }
+				.map { name -> ClusteredTestDetails(name, name, null, null) }
 		)
 
 		testRun.prioritizedClusters?.forEach { cluster ->
diff --git a/teamscale-client/src/main/kotlin/com/teamscale/client/ClusteredTestDetails.kt b/teamscale-client/src/main/kotlin/com/teamscale/client/ClusteredTestDetails.kt
@@ -27,12 +27,6 @@ class ClusteredTestDetails @JsonCreator constructor(
 	 */
 	@param:JsonProperty(
 		"clusterId"
-	) var clusterId: String?,
-	/**
-	 * The partition for the cluster this test should be prioritized within and the result will be uploaded to.
-	 */
-	@param:JsonProperty(
-		"partition"
-	) var partition: String?
+	) var clusterId: String?
 ) : TestDetails(uniformPath, sourcePath, content)
 
diff --git a/teamscale-client/src/main/kotlin/com/teamscale/client/TeamscaleClient.kt b/teamscale-client/src/main/kotlin/com/teamscale/client/TeamscaleClient.kt
@@ -81,7 +81,7 @@ open class TeamscaleClient {
 	 */
 	@Throws(IOException::class)
 	open fun getImpactedTests(
-		availableTests: List<ClusteredTestDetails>?,
+		availableTests: List<TestWithClusterId>?,
 		baseline: String?,
 		baselineRevision: String?,
 		endCommit: CommitDescriptor?,
@@ -134,7 +134,7 @@ open class TeamscaleClient {
 	 */
 	@Throws(IOException::class)
 	private fun getImpactedTests(
-		availableTests: List<ClusteredTestDetails>?,
+		availableTests: List<TestWithClusterId>?,
 		baseline: String?,
 		baselineRevision: String?,
 		endCommit: CommitDescriptor?,
@@ -159,12 +159,9 @@ open class TeamscaleClient {
 				).execute()
 			)
 		} else {
-			val availableTestsMap = availableTests.map { clusteredTestDetails ->
-				TestWithClusterId.fromClusteredTestDetails(clusteredTestDetails)
-			}
 			service.getImpactedTests(
 				projectId, baseline, baselineRevision, endCommit, endRevision, repository, partitions,
-				includeNonImpacted, includeFailedAndSkipped, ensureProcessed, includeAddedTests, availableTestsMap
+				includeNonImpacted, includeFailedAndSkipped, ensureProcessed, includeAddedTests, availableTests
 			).execute()
 		}
 	}
diff --git a/teamscale-client/src/main/kotlin/com/teamscale/client/TestWithClusterId.kt b/teamscale-client/src/main/kotlin/com/teamscale/client/TestWithClusterId.kt
@@ -18,20 +18,20 @@ class TestWithClusterId @JsonCreator constructor(
 	/**
 	 * The partition of the test.
 	 */
-	@param:JsonProperty("partition") val partition: String?,
+	@param:JsonProperty("partition") val partition: String,
 	/**
 	 * A unique identifier for the cluster this test should be prioritized within. May not be null.
 	 */
-	@param:JsonProperty("clusterId") val clusterId: String?
+	@param:JsonProperty("clusterId") val clusterId: String
 ) {
 	companion object {
 		/**
 		 * Creates a #TestWithClusterId from a #ClusteredTestDetails object.
 		 */
-		fun fromClusteredTestDetails(clusteredTestDetails: ClusteredTestDetails) =
+		fun fromClusteredTestDetails(clusteredTestDetails: ClusteredTestDetails, partition: String) =
 			TestWithClusterId(
 				clusteredTestDetails.uniformPath, clusteredTestDetails.content,
-				clusteredTestDetails.partition, clusteredTestDetails.clusterId
+				partition, clusteredTestDetails.clusterId ?: clusteredTestDetails.uniformPath
 			)
 	}
 }
