Skip to content

Commit

Permalink
Fix TestNG tracing for parameterized tests that modify parameters (#7226
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@nikita-tkachenko-datadog
nikita-tkachenko-datadog committed Jun 26, 2024
1 parent 9f7d0c2 commit b6b3c7f
Show file tree
Hide file tree
Showing 18 changed files with 398 additions and 58 deletions.
17 changes: 8 additions & 9 deletions ...gent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilitySystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import java.nio.file.Paths;
import java.util.List;
import java.util.function.Predicate;
import javax.annotation.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -162,22 +163,20 @@ private TestEventsHandlerFactory(
}
}

@Override
public <SuiteKey, TestKey> TestEventsHandler<SuiteKey, TestKey> create(String component) {
return create(
component, new ConcurrentHashMapContextStore<>(), new ConcurrentHashMapContextStore<>());
}

@Override
public <SuiteKey, TestKey> TestEventsHandler<SuiteKey, TestKey> create(
String component,
ContextStore<SuiteKey, DDTestSuite> suiteStore,
ContextStore<TestKey, DDTest> testStore) {
@Nullable ContextStore<SuiteKey, DDTestSuite> suiteStore,
@Nullable ContextStore<TestKey, DDTest> testStore) {
TestFrameworkSession testSession =
sessionFactory.startSession(repoServices.moduleName, component, null);
TestFrameworkModule testModule = testSession.testModuleStart(repoServices.moduleName, null);
return new TestEventsHandlerImpl<>(
services.metricCollector, testSession, testModule, suiteStore, testStore);
services.metricCollector,
testSession,
testModule,
suiteStore != null ? suiteStore : new ConcurrentHashMapContextStore<>(),
testStore != null ? testStore : new ConcurrentHashMapContextStore<>());
}
}

Expand Down
9 changes: 3 additions & 6 deletions ...src/testFixtures/groovy/datadog/trace/civisibility/CiVisibilityInstrumentationTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,16 +184,13 @@ abstract class CiVisibilityInstrumentationTest extends AgentTestRunner {
this.metricCollector = metricCollector
}

@Override
<SuiteKey, TestKey> TestEventsHandler<SuiteKey, TestKey> create(String component) {
return create(component, new ConcurrentHashMapContextStore<>(), new ConcurrentHashMapContextStore())
}

@Override
<SuiteKey, TestKey> TestEventsHandler<SuiteKey, TestKey> create(String component, ContextStore<SuiteKey, DDTestSuite> suiteStore, ContextStore<TestKey, DDTest> testStore) {
TestFrameworkSession testSession = testFrameworkSessionFactory.startSession(dummyModule, component, null)
TestFrameworkModule testModule = testSession.testModuleStart(dummyModule, null)
new TestEventsHandlerImpl(metricCollector, testSession, testModule, suiteStore, testStore)
new TestEventsHandlerImpl(metricCollector, testSession, testModule,
suiteStore != null ? suiteStore : new ConcurrentHashMapContextStore<>(),
testStore != null ? testStore : new ConcurrentHashMapContextStore<>())
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...unit-4.10/src/main/java/datadog/trace/instrumentation/junit4/TestEventsHandlerHolder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public abstract class TestEventsHandlerHolder {
}

public static void start() {
TEST_EVENTS_HANDLER = InstrumentationBridge.createTestEventsHandler("junit");
TEST_EVENTS_HANDLER = InstrumentationBridge.createTestEventsHandler("junit", null, null);
}

public static void stop() {
Expand Down
2 changes: 1 addition & 1 deletion ...on/karate/src/main/java/datadog/trace/instrumentation/karate/TestEventsHandlerHolder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public abstract class TestEventsHandlerHolder {
}

public static void start() {
TEST_EVENTS_HANDLER = InstrumentationBridge.createTestEventsHandler("karate");
TEST_EVENTS_HANDLER = InstrumentationBridge.createTestEventsHandler("karate", null, null);
}

public static void stop() {
Expand Down
2 changes: 1 addition & 1 deletion ...mentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/RunContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public static void destroy(int runStamp) {

private final int runStamp;
private final TestEventsHandler<TestSuiteDescriptor, TestDescriptor> eventHandler =
InstrumentationBridge.createTestEventsHandler("scalatest");
InstrumentationBridge.createTestEventsHandler("scalatest", null, null);
private final java.util.Set<TestIdentifier> skippedTests = ConcurrentHashMap.newKeySet();
private final java.util.Set<TestIdentifier> unskippableTests = ConcurrentHashMap.newKeySet();
private final java.util.Map<TestIdentifier, TestRetryPolicy> retryPolicies =
Expand Down
17 changes: 13 additions & 4 deletions ...on/testng/src/main/java/datadog/trace/instrumentation/testng/TestEventsHandlerHolder.java
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
package datadog.trace.instrumentation.testng;

import datadog.trace.api.civisibility.DDTest;
import datadog.trace.api.civisibility.InstrumentationBridge;
import datadog.trace.api.civisibility.events.TestDescriptor;
import datadog.trace.api.civisibility.events.TestEventsHandler;
import datadog.trace.api.civisibility.events.TestSuiteDescriptor;
import datadog.trace.bootstrap.ContextStore;
import datadog.trace.util.AgentThreadFactory;
import org.testng.ITestResult;

public abstract class TestEventsHandlerHolder {

public static volatile TestEventsHandler<TestSuiteDescriptor, TestDescriptor> TEST_EVENTS_HANDLER;
public static volatile TestEventsHandler<TestSuiteDescriptor, ITestResult> TEST_EVENTS_HANDLER;

private static ContextStore<ITestResult, DDTest> TEST_STORE;

static {
start();
Runtime.getRuntime()
.addShutdownHook(
AgentThreadFactory.newAgentThread(
Expand All @@ -20,8 +23,14 @@ public abstract class TestEventsHandlerHolder {
false));
}

public static synchronized void setContextStore(ContextStore<ITestResult, DDTest> testStore) {
if (TEST_STORE == null) {
TEST_STORE = testStore;
}
}

public static void start() {
TEST_EVENTS_HANDLER = InstrumentationBridge.createTestEventsHandler("testng");
TEST_EVENTS_HANDLER = InstrumentationBridge.createTestEventsHandler("testng", null, TEST_STORE);
}

public static void stop() {
Expand Down
17 changes: 17 additions & 0 deletions ...tion/testng/src/main/java/datadog/trace/instrumentation/testng/TestNGInstrumentation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,17 @@
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.InstrumenterModule;
import datadog.trace.api.Config;
import datadog.trace.api.civisibility.DDTest;
import datadog.trace.bootstrap.ContextStore;
import datadog.trace.bootstrap.InstrumentationContext;
import datadog.trace.instrumentation.testng.retry.RetryAnnotationTransformer;
import java.util.Collections;
import java.util.Map;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import org.testng.ITestListener;
import org.testng.ITestNGListener;
import org.testng.ITestResult;
import org.testng.TestNG;
import org.testng.annotations.DataProvider;

Expand Down Expand Up @@ -43,6 +49,12 @@ public String[] helperClassNames() {
};
}

@Override
public Map<String, String> contextStore() {
return Collections.singletonMap(
"org.testng.ITestResult", "datadog.trace.api.civisibility.DDTest");
}

public static class TestNGAdvice {
@Advice.OnMethodExit
public static void addTracingListener(@Advice.This final TestNG testNG) {
Expand All @@ -52,6 +64,11 @@ public static void addTracingListener(@Advice.This final TestNG testNG) {
}
}

ContextStore<ITestResult, DDTest> contextStore =
InstrumentationContext.get(ITestResult.class, DDTest.class);
TestEventsHandlerHolder.setContextStore(contextStore);
TestEventsHandlerHolder.start();

final TracingListener tracingListener = new TracingListener();
testNG.addListener((ITestNGListener) tracingListener);

Expand Down
11 changes: 0 additions & 11 deletions ...nstrumentation/testng/src/main/java/datadog/trace/instrumentation/testng/TestNGUtils.java
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package datadog.trace.instrumentation.testng;

import datadog.trace.api.civisibility.config.TestIdentifier;
import datadog.trace.api.civisibility.events.TestDescriptor;
import datadog.trace.api.civisibility.events.TestSuiteDescriptor;
import datadog.trace.util.Strings;
import java.io.InputStream;
Expand Down Expand Up @@ -247,16 +246,6 @@ public static TestIdentifier toTestIdentifier(ITestResult result) {
return new TestIdentifier(testSuiteName, testName, testParameters, null);
}

public static TestDescriptor toTestDescriptor(ITestResult result) {
String testSuiteName = result.getInstanceName();
IClass iTestClass = result.getTestClass();
Class<?> testClass = iTestClass != null ? iTestClass.getRealClass() : null;
String testName =
(result.getName() != null) ? result.getName() : result.getMethod().getMethodName();
String parameters = TestNGUtils.getParameters(result);
return new TestDescriptor(testSuiteName, testClass, testName, parameters, result);
}

public static TestSuiteDescriptor toSuiteDescriptor(ITestClass testClass) {
String testSuiteName = testClass.getName();
Class<?> testSuiteClass = testClass.getRealClass();
Expand Down
19 changes: 7 additions & 12 deletions ...umentation/testng/src/main/java/datadog/trace/instrumentation/testng/TracingListener.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package datadog.trace.instrumentation.testng;

import datadog.trace.api.civisibility.events.TestDescriptor;
import datadog.trace.api.civisibility.events.TestSuiteDescriptor;
import datadog.trace.api.civisibility.telemetry.tag.TestFrameworkInstrumentation;
import datadog.trace.instrumentation.testng.retry.RetryAnalyzer;
Expand Down Expand Up @@ -75,7 +74,6 @@ public void onConfigurationSkip(ITestResult result) {
public void onTestStart(final ITestResult result) {
TestSuiteDescriptor suiteDescriptor =
TestNGUtils.toSuiteDescriptor(result.getMethod().getTestClass());
TestDescriptor testDescriptor = TestNGUtils.toTestDescriptor(result);
String testSuiteName = result.getInstanceName();
String testName =
(result.getName() != null) ? result.getName() : result.getMethod().getMethodName();
Expand All @@ -86,7 +84,7 @@ public void onTestStart(final ITestResult result) {
String testMethodName = testMethod != null ? testMethod.getName() : null;
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestStart(
suiteDescriptor,
testDescriptor,
result,
testSuiteName,
testName,
FRAMEWORK_NAME,
Expand All @@ -110,16 +108,14 @@ private boolean isRetry(final ITestResult result) {

@Override
public void onTestSuccess(final ITestResult result) {
TestDescriptor testDescriptor = TestNGUtils.toTestDescriptor(result);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFinish(testDescriptor);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFinish(result);
}

@Override
public void onTestFailure(final ITestResult result) {
TestDescriptor testDescriptor = TestNGUtils.toTestDescriptor(result);
Throwable throwable = result.getThrowable();
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFailure(testDescriptor, throwable);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFinish(testDescriptor);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFailure(result, throwable);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFinish(result);
}

@Override
Expand All @@ -129,19 +125,18 @@ public void onTestFailedButWithinSuccessPercentage(final ITestResult result) {

@Override
public void onTestSkipped(final ITestResult result) {
TestDescriptor testDescriptor = TestNGUtils.toTestDescriptor(result);
Throwable throwable = result.getThrowable();
if (TestNGUtils.wasRetried(result)) {
// TestNG reports tests retried with IRetryAnalyzer as skipped,
// this is done to avoid failing the build when retrying tests.
// We want to report such tests as failed to Datadog,
// to provide more accurate data (and to enable flakiness detection)
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFailure(testDescriptor, throwable);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFailure(result, throwable);
} else {
// Typically the way of skipping a TestNG test is throwing a SkipException
String reason = throwable != null ? throwable.getMessage() : null;
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestSkip(testDescriptor, reason);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestSkip(result, reason);
}
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFinish(testDescriptor);
TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFinish(result);
}
}
2 changes: 2 additions & 0 deletions ...ion/testng/src/testFixtures/groovy/datadog/trace/instrumentation/testng/TestNGTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import org.example.TestFailedThenSucceed
import org.example.TestFailedWithSuccessPercentage
import org.example.TestInheritance
import org.example.TestParameterized
import org.example.TestParameterizedModifiesParams
import org.example.TestSkipped
import org.example.TestSkippedClass
import org.example.TestSkippedNested
Expand Down Expand Up @@ -47,6 +48,7 @@ abstract class TestNGTest extends CiVisibilityInstrumentationTest {
"test-error" | [TestError] | 2
"test-skipped" | [TestSkipped] | 2
"test-parameterized" | [TestParameterized] | 3
"test-parameterized-modifies-params" | [TestParameterizedModifiesParams] | 2
"test-success-with-groups" | [TestSucceedGroups] | 2
"test-class-skipped" | [TestSkippedClass] | 3
"test-success-and-skipped" | [TestSucceedAndSkipped] | 3
Expand Down
23 changes: 23 additions & 0 deletions ...rumentation/testng/src/testFixtures/java/org/example/TestParameterizedModifiesParams.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.example;

import static org.testng.AssertJUnit.assertEquals;

import java.util.HashSet;
import java.util.Set;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

/** Inspired by a real-world example */
public class TestParameterizedModifiesParams {

@DataProvider(name = "dataProvider")
public static Object[][] data() {
return new Object[][] {{"I will modify this set", new HashSet<>()}};
}

@Test(dataProvider = "dataProvider")
public void parameterized_test_succeed(final String str, final Set<String> set) {
set.add("why not");
assertEquals(1, set.size());
}
}
1 change: 1 addition & 0 deletions ...ation/testng/testng-6/src/test/resources/test-parameterized-modifies-params/coverages.ftl
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[ ]
Loading

0 comments on commit b6b3c7f

Please sign in to comment.