From 0807850bf7a6c7cd366228ce0535ff70b2513eac Mon Sep 17 00:00:00 2001 From: Florian Dreier Date: Thu, 10 Apr 2025 11:24:18 +0200 Subject: [PATCH] TS-42472 Fixed validation for revision option in testwise mode --- CHANGELOG.md | 1 + .../jacoco/agent/options/AgentOptions.java | 19 +++--- .../CoverageToTeamscaleStrategy.java | 27 +++------ .../TestEventHandlerStrategyBase.java | 38 ++++++------ .../agent/options/AgentOptionsParserTest.java | 27 ++++++++- .../CoverageToTeamscaleStrategyTest.java | 60 ++++++++++--------- 6 files changed, 98 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03a83d690..2bd8005ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ We use [semantic versioning](http://semver.org/): - [feature] _agent_: Experimental support for Java 25 class files - [fix] _agent_: Reported wrong version number in docker container - [fix] _agent_: Erroneous log messages were printed about unmatched probes +- [fix] _agent_: Couldn't receive impacted tests when revision is provided instead of commit # 35.0.3 - [fix] _teamscale-gradle-plugin_: `IllegalArgumentException`: Can't execute request without discovering it first diff --git a/agent/src/main/java/com/teamscale/jacoco/agent/options/AgentOptions.java b/agent/src/main/java/com/teamscale/jacoco/agent/options/AgentOptions.java index f0d6337d4..05962a32f 100644 --- a/agent/src/main/java/com/teamscale/jacoco/agent/options/AgentOptions.java +++ b/agent/src/main/java/com/teamscale/jacoco/agent/options/AgentOptions.java @@ -377,17 +377,22 @@ private void validateSapNetWeaverConfig(Validator validator) { } private void validateTestwiseCoverageConfig(Validator validator) { - validator.isFalse( - !(testwiseCoverageMode == ETestwiseCoverageMode.DISK) && httpServerPort == null, - "You use 'mode' 'TESTWISE' but did use neither 'http-server-port' nor dumping to disk!" + - " One of them is required!"); + validator.isTrue(httpServerPort != null, + "You use 'mode=testwise' but did not specify the required option 'http-server-port'!"); - validator.isFalse(testwiseCoverageMode == ETestwiseCoverageMode.TEAMSCALE_UPLOAD - && !teamscaleServer.isConfiguredForSingleProjectTeamscaleUpload(), + validator.isTrue(testwiseCoverageMode != ETestwiseCoverageMode.TEAMSCALE_UPLOAD + || teamscaleServer.isConfiguredForSingleProjectTeamscaleUpload(), "You use 'tia-mode=teamscale-upload' but did not set all required 'teamscale-' fields to facilitate" + " a connection to Teamscale!"); - } + validator.isTrue( + testwiseCoverageMode != ETestwiseCoverageMode.TEAMSCALE_UPLOAD || teamscaleServer.hasCommitOrRevision(), + "You use 'tia-mode=teamscale-upload' but did not provide a revision or commit via the agent's '" + TeamscaleConfig.TEAMSCALE_REVISION_OPTION + "', '" + + TeamscaleConfig.TEAMSCALE_REVISION_MANIFEST_JAR_OPTION + "', '" + TeamscaleConfig.TEAMSCALE_COMMIT_OPTION + + "', '" + TeamscaleConfig.TEAMSCALE_COMMIT_MANIFEST_JAR_OPTION + "' or '" + + AgentOptions.GIT_PROPERTIES_JAR_OPTION + "' option." + + " Auto-detecting the git.properties is currently not supported in this mode."); + } /** * Creates a {@link TeamscaleClient} based on the agent options. Returns null if the user did not fully configure a diff --git a/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategy.java b/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategy.java index 808fd23c2..3c9b98b5e 100644 --- a/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategy.java +++ b/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategy.java @@ -1,36 +1,25 @@ package com.teamscale.jacoco.agent.testimpact; -import java.io.File; -import java.io.IOException; -import java.util.List; - -import org.conqat.lib.commons.filesystem.FileSystemUtils; - import com.teamscale.client.EReportFormat; import com.teamscale.jacoco.agent.JacocoRuntimeController; import com.teamscale.jacoco.agent.options.AgentOptions; -import com.teamscale.jacoco.agent.upload.teamscale.TeamscaleConfig; import com.teamscale.report.testwise.jacoco.JaCoCoTestwiseReportGenerator; +import org.conqat.lib.commons.filesystem.FileSystemUtils; + +import java.io.File; +import java.io.IOException; +import java.util.List; /** * Strategy that records test-wise coverage and uploads the resulting report to Teamscale. Also handles the - * {@link #testRunStart(List, boolean, boolean, boolean, String, String)} event by retrieving tests to run from Teamscale. + * {@link #testRunStart(List, boolean, boolean, boolean, String, String)} event by retrieving tests to run from + * Teamscale. */ public class CoverageToTeamscaleStrategy extends CoverageToJsonStrategyBase { public CoverageToTeamscaleStrategy(JacocoRuntimeController controller, AgentOptions agentOptions, - JaCoCoTestwiseReportGenerator reportGenerator) { + JaCoCoTestwiseReportGenerator reportGenerator) { super(controller, agentOptions, reportGenerator); - - if (!agentOptions.getTeamscaleServerOptions().hasCommitOrRevision()) { - throw new UnsupportedOperationException( - "You must provide a commit or revision via the agent's '" + TeamscaleConfig.TEAMSCALE_COMMIT_OPTION + - "', '" + TeamscaleConfig.TEAMSCALE_COMMIT_MANIFEST_JAR_OPTION + "', '" + - TeamscaleConfig.TEAMSCALE_REVISION_OPTION + "' or '" + - AgentOptions.GIT_PROPERTIES_JAR_OPTION + "' option." + - " Auto-detecting the git.properties does not work since we need the commit before any code" + - " has been profiled in order to obtain the prioritized test cases from the TIA."); - } } @Override 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 index 8d1b811eb..cc9faa342 100644 --- a/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/TestEventHandlerStrategyBase.java +++ b/agent/src/main/java/com/teamscale/jacoco/agent/testimpact/TestEventHandlerStrategyBase.java @@ -1,25 +1,23 @@ package com.teamscale.jacoco.agent.testimpact; -import java.io.IOException; -import java.util.Collections; -import java.util.List; - -import org.slf4j.Logger; - import com.teamscale.client.ClusteredTestDetails; import com.teamscale.client.HttpUtils; import com.teamscale.client.PrioritizableTestCluster; import com.teamscale.client.TeamscaleClient; import com.teamscale.jacoco.agent.JacocoRuntimeController; +import com.teamscale.jacoco.agent.logging.LoggingUtils; import com.teamscale.jacoco.agent.options.AgentOptions; import com.teamscale.jacoco.agent.upload.teamscale.TeamscaleConfig; -import com.teamscale.jacoco.agent.logging.LoggingUtils; import com.teamscale.report.testwise.jacoco.cache.CoverageGenerationException; import com.teamscale.report.testwise.model.TestExecution; import com.teamscale.report.testwise.model.TestInfo; - +import org.slf4j.Logger; import retrofit2.Response; +import java.io.IOException; +import java.util.Collections; +import java.util.List; + /** Base class for strategies to handle test events. */ public abstract class TestEventHandlerStrategyBase { @@ -63,7 +61,7 @@ public void testStart(String test) { * as a json response. */ public TestInfo testEnd(String test, - TestExecution testExecution) throws JacocoRuntimeController.DumpException, CoverageGenerationException { + TestExecution testExecution) throws JacocoRuntimeController.DumpException, CoverageGenerationException { if (testExecution != null) { testExecution.uniformPath = test; if (startTimestamp != -1) { @@ -86,9 +84,9 @@ public TestInfo testEnd(String test, * @throws UnsupportedOperationException if the user did not properly configure the {@link #teamscaleClient}. */ public List testRunStart(List 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 { int availableTestCount = 0; if (availableTests != null) { availableTestCount = availableTests.size(); @@ -99,13 +97,14 @@ public List testRunStart(List av Response> response = teamscaleClient .getImpactedTests(availableTests, baseline, baselineRevision, - agentOptions.getTeamscaleServerOptions().commit, agentOptions.getTeamscaleServerOptions().revision, + agentOptions.getTeamscaleServerOptions().commit, + agentOptions.getTeamscaleServerOptions().revision, agentOptions.getTeamscaleServerOptions().repository, Collections.singletonList(agentOptions.getTeamscaleServerOptions().partition), includeNonImpactedTests, includeAddedTests, includeFailedAndSkipped); if (response.isSuccessful()) { List prioritizableTestClusters = response.body(); - logger.debug("Teamscale suggested these tests: {}", prioritizableTestClusters.toString()); + logger.debug("Teamscale suggested these tests: {}", prioritizableTestClusters); return prioritizableTestClusters; } else { String responseBody = HttpUtils.getErrorBodyStringSafe(response); @@ -121,11 +120,14 @@ private void validateConfiguration() { " Thus, you cannot use the agent to retrieve impacted tests via the testrun/start REST endpoint." + " Please use the 'teamscale-' agent parameters to configure a Teamscale connection."); } - if (agentOptions.getTeamscaleServerOptions().commit == null) { + if (!agentOptions.getTeamscaleServerOptions().hasCommitOrRevision()) { throw new UnsupportedOperationException( - "You did not provide a '" + TeamscaleConfig.TEAMSCALE_COMMIT_OPTION + "' or '" + - TeamscaleConfig.TEAMSCALE_COMMIT_MANIFEST_JAR_OPTION + "'. '" + - TeamscaleConfig.TEAMSCALE_REVISION_OPTION + "' is not sufficient to retrieve impacted tests."); + "You must provide a revision or commit via the agent's '" + TeamscaleConfig.TEAMSCALE_REVISION_OPTION + "', '" + + TeamscaleConfig.TEAMSCALE_REVISION_MANIFEST_JAR_OPTION + "', '" + TeamscaleConfig.TEAMSCALE_COMMIT_OPTION + + "', '" + TeamscaleConfig.TEAMSCALE_COMMIT_MANIFEST_JAR_OPTION + "' or '" + + AgentOptions.GIT_PROPERTIES_JAR_OPTION + "' option." + + " Auto-detecting the git.properties does not work since we need the commit before any code" + + " has been profiled in order to obtain the prioritized test cases from the TIA."); } } diff --git a/agent/src/test/java/com/teamscale/jacoco/agent/options/AgentOptionsParserTest.java b/agent/src/test/java/com/teamscale/jacoco/agent/options/AgentOptionsParserTest.java index 7dd706131..1ccf18140 100644 --- a/agent/src/test/java/com/teamscale/jacoco/agent/options/AgentOptionsParserTest.java +++ b/agent/src/test/java/com/teamscale/jacoco/agent/options/AgentOptionsParserTest.java @@ -64,7 +64,7 @@ public void testUploadMethodRecognition() throws Exception { assertThat(parseAndThrow(basicTeamscaleOptions + ",teamscale-project=proj") .determineUploadMethod()).isEqualTo(AgentOptions.EUploadMethod.TEAMSCALE_SINGLE_PROJECT); assertThat(parseAndThrow( - basicTeamscaleOptions + ",sap-nwdi-applications=com.package.MyClass:projectId;com.company.Main:project") + basicTeamscaleOptions + ",sap-nwdi-applications=com.package.MyClass:projectId;com.company.Main:project") .determineUploadMethod()) .isEqualTo(AgentOptions.EUploadMethod.SAP_NWDI_TEAMSCALE); } @@ -89,7 +89,7 @@ public void testUploadMethodRecognitionWithTeamscaleProperties() throws Exceptio assertThat(parseAndThrow(basicTeamscaleOptions + ",teamscale-project=proj") .determineUploadMethod()).isEqualTo(AgentOptions.EUploadMethod.TEAMSCALE_SINGLE_PROJECT); assertThat(parseAndThrow( - basicTeamscaleOptions + ",sap-nwdi-applications=com.package.MyClass:projectId;com.company.Main:project") + basicTeamscaleOptions + ",sap-nwdi-applications=com.package.MyClass:projectId;com.company.Main:project") .determineUploadMethod()) .isEqualTo(AgentOptions.EUploadMethod.SAP_NWDI_TEAMSCALE); } @@ -220,6 +220,26 @@ public void revisionOrCommitRequireProject() { ).hasMessageContaining("you did not provide the 'teamscale-project'"); } + @Test + public void testTeamscaleUploadRequiresRevisionOrCommit() { + String teamscaleBaseOptions = "teamscale-server-url=teamscale.com,teamscale-user=user,teamscale-access-token=token,teamscale-partition=p,teamscale-project=proj,mode=testwise,http-server-port=8123,"; + assertThatThrownBy( + () -> parseAndThrow( + teamscaleBaseOptions + "tia-mode=teamscale-upload") + ).hasMessageContaining("You use 'tia-mode=teamscale-upload' but did not provide a revision or commit"); + + assertThatCode( + () -> parseAndThrow( + teamscaleBaseOptions + "tia-mode=teamscale-upload,teamscale-revision=12345") + ).doesNotThrowAnyException(); + + assertThatCode( + () -> parseAndThrow( + teamscaleBaseOptions + "tia-mode=teamscale-upload,teamscale-commit=master:HEAD") + ).doesNotThrowAnyException(); + } + + @Test public void environmentConfigIdDoesNotExist() { mockWebServer.enqueue(new MockResponse().setResponseCode(404).setBody("invalid-config-id does not exist")); @@ -245,7 +265,8 @@ public void mustPreserveDefaultExcludes() throws Exception { @Test public void teamscalePropertiesCredentialsUsedAsDefaultButOverridable() throws Exception { - assertThat(parseAndThrow(new AgentOptionsParser(new CommandLineLogger(), null, null, teamscaleCredentials), "teamscale-project=p,teamscale-partition=p").teamscaleServer.userName).isEqualTo( + assertThat(parseAndThrow(new AgentOptionsParser(new CommandLineLogger(), null, null, teamscaleCredentials), + "teamscale-project=p,teamscale-partition=p").teamscaleServer.userName).isEqualTo( "user"); assertThat(parseAndThrow(new AgentOptionsParser(new CommandLineLogger(), null, null, teamscaleCredentials), "teamscale-project=p,teamscale-partition=p,teamscale-user=user2").teamscaleServer.userName).isEqualTo( 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 index 14233b1f7..2cc3f85a6 100644 --- a/agent/src/test/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategyTest.java +++ b/agent/src/test/java/com/teamscale/jacoco/agent/testimpact/CoverageToTeamscaleStrategyTest.java @@ -1,24 +1,5 @@ package com.teamscale.jacoco.agent.testimpact; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.matches; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import java.io.File; -import java.io.IOException; -import java.util.Collections; -import java.util.List; - -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.jupiter.api.io.TempDir; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - import com.teamscale.client.ClusteredTestDetails; import com.teamscale.client.CommitDescriptor; import com.teamscale.client.EReportFormat; @@ -34,10 +15,29 @@ import com.teamscale.report.testwise.model.TestwiseCoverage; import com.teamscale.report.testwise.model.builder.FileCoverageBuilder; import com.teamscale.report.testwise.model.builder.TestCoverageBuilder; - import okhttp3.HttpUrl; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import retrofit2.Response; +import java.io.File; +import java.io.IOException; +import java.util.Collections; +import java.util.List; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.matches; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + @ExtendWith(MockitoExtension.class) public class CoverageToTeamscaleStrategyTest { @@ -55,7 +55,7 @@ public class CoverageToTeamscaleStrategyTest { @Test public void shouldRecordCoverageForTestsEvenIfNotProvidedAsAvailableTest() throws Exception { - AgentOptions options = mockOptions(); + AgentOptions options = mockOptions(false); CoverageToTeamscaleStrategy strategy = new CoverageToTeamscaleStrategy(controller, options, reportGenerator); TestwiseCoverage testwiseCoverage = getDummyTestwiseCoverage("mytest"); @@ -71,18 +71,20 @@ public void shouldRecordCoverageForTestsEvenIfNotProvidedAsAvailableTest() throw any(), any(), any(), any(), any()); } - @Test - public void testValidCallSequence() throws Exception { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testValidCallSequence(boolean useRevision) throws Exception { List clusters = Collections .singletonList(new PrioritizableTestCluster("cluster", Collections.singletonList(new PrioritizableTest("mytest")))); - when(client.getImpactedTests(any(), any(), any(), any(), any(), any(), any(), anyBoolean(), anyBoolean(), anyBoolean())).thenReturn( + when(client.getImpactedTests(any(), any(), any(), any(), any(), any(), any(), anyBoolean(), anyBoolean(), + anyBoolean())).thenReturn( Response.success(clusters)); TestwiseCoverage testwiseCoverage = getDummyTestwiseCoverage("mytest"); when(reportGenerator.convert(any(File.class))).thenReturn(testwiseCoverage); - AgentOptions options = mockOptions(); + AgentOptions options = mockOptions(useRevision); JacocoRuntimeController controller = mock(JacocoRuntimeController.class); CoverageToTeamscaleStrategy strategy = new CoverageToTeamscaleStrategy(controller, options, reportGenerator); @@ -111,13 +113,17 @@ protected static TestwiseCoverage getDummyTestwiseCoverage(String test) { return testwiseCoverage; } - private AgentOptions mockOptions() throws IOException { + private AgentOptions mockOptions(boolean useRevision) throws IOException { AgentOptions options = mock(AgentOptions.class); when(options.createTeamscaleClient(true)).thenReturn(client); when(options.createNewFileInOutputDirectory(any(), any())).thenReturn(new File(tempDir, "test")); TeamscaleServer server = new TeamscaleServer(); - server.commit = new CommitDescriptor("branch", "12345"); + if (useRevision) { + server.revision = "rev1"; + } else { + server.commit = new CommitDescriptor("branch", "12345"); + } server.url = HttpUrl.get("http://doesnt-exist.io"); server.userName = "build"; server.userAccessToken = "token";