Skip to content

Commit 30789d4

Browse files
nikita-tkachenko-datadognikita-tkachenko-datadog
authored
Implement flaky test retries for JUnit 5.3 (DataDog#6332)
1 parent 66e927b commit 30789d4

File tree

109 files changed

+4375
-744
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

109 files changed

+4375
-744
lines changed

‎dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/ContextStore.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,18 @@ interface Factory<C> {
2222
C create();
2323
}
2424

25+
/**
26+
* Factory interface to create context instances using context key instances
27+
*
28+
* @param <K> context key type
29+
* @param <C> context value type
30+
*/
31+
interface KeyAwareFactory<K, C> {
32+
33+
/** @return new context instance */
34+
C create(K key);
35+
}
36+
2537
/**
2638
* Get context given the key
2739
*
@@ -57,6 +69,16 @@ interface Factory<C> {
5769
*/
5870
C putIfAbsent(K key, Factory<C> contextFactory);
5971

72+
/**
73+
* Put new context instance if key is absent. Uses context factory to avoid creating objects if
74+
* not needed.
75+
*
76+
* @param key key to use
77+
* @param contextFactory factory instance to produce new context object
78+
* @return old instance if it was present, or new instance
79+
*/
80+
C computeIfAbsent(K key, KeyAwareFactory<K, C> contextFactory);
81+
6082
/**
6183
* Removes the existing value for key and return it.
6284
*

‎dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/FieldBackedContextStore.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,21 +51,26 @@ public Object putIfAbsent(final Object key, final Object context) {
5151

5252
@Override
5353
public Object putIfAbsent(final Object key, final Factory<Object> contextFactory) {
54+
return computeIfAbsent(key, k -> contextFactory.create());
55+
}
56+
57+
@Override
58+
public Object computeIfAbsent(Object key, KeyAwareFactory<Object, Object> contextFactory) {
5459
if (key instanceof FieldBackedContextAccessor) {
5560
final FieldBackedContextAccessor accessor = (FieldBackedContextAccessor) key;
5661
Object existingContext = accessor.$get$__datadogContext$(storeId);
5762
if (null == existingContext) {
5863
synchronized (accessor) {
5964
existingContext = accessor.$get$__datadogContext$(storeId);
6065
if (null == existingContext) {
61-
existingContext = contextFactory.create();
66+
existingContext = contextFactory.create(key);
6267
accessor.$put$__datadogContext$(storeId, existingContext);
6368
}
6469
}
6570
}
6671
return existingContext;
6772
} else {
68-
return weakStore().putIfAbsent(key, contextFactory);
73+
return weakStore().computeIfAbsent(key, contextFactory);
6974
}
7075
}
7176

‎dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapContextStore.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ public V putIfAbsent(final K key, final V context) {
5555

5656
@Override
5757
public V putIfAbsent(final K key, final Factory<V> contextFactory) {
58+
return computeIfAbsent(key, k -> contextFactory.create());
59+
}
60+
61+
@Override
62+
public V computeIfAbsent(K key, KeyAwareFactory<K, V> contextFactory) {
5863
V existingContext = get(key);
5964
if (null == existingContext) {
6065
// This whole part with using synchronized is only because
@@ -66,7 +71,7 @@ public V putIfAbsent(final K key, final Factory<V> contextFactory) {
6671
synchronized (map) {
6772
existingContext = get(key);
6873
if (null == existingContext) {
69-
existingContext = contextFactory.create();
74+
existingContext = contextFactory.create(key);
7075
put(key, existingContext);
7176
}
7277
}

‎dd-java-agent/agent-ci-visibility/build.gradle‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ excludedClassesCoverage += [
3939
"datadog.trace.civisibility.config.ConfigurationApiImpl.MultiEnvelopeDto",
4040
"datadog.trace.civisibility.config.ConfigurationApi.1",
4141
"datadog.trace.civisibility.config.ModuleExecutionSettingsFactoryImpl",
42-
"datadog.trace.civisibility.config.SkippableTestsSerializer",
42+
"datadog.trace.civisibility.config.TestIdentifierSerializer",
4343
"datadog.trace.civisibility.coverage.CoverageUtils",
4444
"datadog.trace.civisibility.coverage.CoverageUtils.RepoIndexFileLocator",
4545
"datadog.trace.civisibility.coverage.ExecutionDataAdapter",
@@ -66,8 +66,10 @@ excludedClassesCoverage += [
6666
"datadog.trace.civisibility.ipc.SignalType",
6767
"datadog.trace.civisibility.ipc.SignalClient",
6868
"datadog.trace.civisibility.ipc.SignalClient.Factory",
69-
"datadog.trace.civisibility.ipc.SkippableTestsResponse",
69+
"datadog.trace.civisibility.ipc.TestDataResponse",
7070
"datadog.trace.civisibility.ipc.TestFramework",
71+
"datadog.trace.civisibility.retry.NeverRetry",
72+
"datadog.trace.civisibility.retry.RetryIfFailed",
7173
"datadog.trace.civisibility.source.index.PackageTree.Node",
7274
"datadog.trace.civisibility.source.index.RepoIndex",
7375
"datadog.trace.civisibility.source.index.RepoIndexBuilder.RepoIndexingFileVisitor",

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/DDBuildSystemSessionImpl.java‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import datadog.trace.api.Config;
44
import datadog.trace.api.DDTags;
55
import datadog.trace.api.civisibility.config.ModuleExecutionSettings;
6-
import datadog.trace.api.civisibility.config.SkippableTest;
6+
import datadog.trace.api.civisibility.config.TestIdentifier;
77
import datadog.trace.bootstrap.instrumentation.api.Tags;
88
import datadog.trace.civisibility.codeowners.Codeowners;
99
import datadog.trace.civisibility.config.JvmInfo;
@@ -18,8 +18,9 @@
1818
import datadog.trace.civisibility.ipc.SignalResponse;
1919
import datadog.trace.civisibility.ipc.SignalServer;
2020
import datadog.trace.civisibility.ipc.SignalType;
21-
import datadog.trace.civisibility.ipc.SkippableTestsRequest;
22-
import datadog.trace.civisibility.ipc.SkippableTestsResponse;
21+
import datadog.trace.civisibility.ipc.TestDataRequest;
22+
import datadog.trace.civisibility.ipc.TestDataResponse;
23+
import datadog.trace.civisibility.ipc.TestDataType;
2324
import datadog.trace.civisibility.source.MethodLinesResolver;
2425
import datadog.trace.civisibility.source.SourcePathResolver;
2526
import datadog.trace.civisibility.source.index.RepoIndex;
@@ -91,7 +92,7 @@ public DDBuildSystemSessionImpl(
9192
signalServer.registerSignalHandler(
9293
SignalType.REPO_INDEX_REQUEST, this::onRepoIndexRequestReceived);
9394
signalServer.registerSignalHandler(
94-
SignalType.SKIPPABLE_TESTS_REQUEST, this::onSkippableTestsRequestReceived);
95+
SignalType.TEST_DATA_REQUEST, this::onTestDataRequestReceived);
9596
signalServer.start();
9697

9798
setTag(Tags.TEST_COMMAND, startCommand);
@@ -127,15 +128,27 @@ private SignalResponse onRepoIndexRequestReceived(RepoIndexRequest request) {
127128
}
128129
}
129130

130-
private SignalResponse onSkippableTestsRequestReceived(
131-
SkippableTestsRequest skippableTestsRequest) {
131+
private SignalResponse onTestDataRequestReceived(TestDataRequest testDataRequest) {
132132
try {
133-
String relativeModulePath = skippableTestsRequest.getRelativeModulePath();
134-
JvmInfo jvmInfo = skippableTestsRequest.getJvmInfo();
133+
String relativeModulePath = testDataRequest.getRelativeModulePath();
134+
JvmInfo jvmInfo = testDataRequest.getJvmInfo();
135135
ModuleExecutionSettings moduleExecutionSettings = getModuleExecutionSettings(jvmInfo);
136-
Collection<SkippableTest> tests =
137-
moduleExecutionSettings.getSkippableTests(relativeModulePath);
138-
return new SkippableTestsResponse(tests);
136+
137+
Collection<TestIdentifier> tests;
138+
TestDataType testDataType = testDataRequest.getTestDataType();
139+
switch (testDataType) {
140+
case SKIPPABLE:
141+
tests = moduleExecutionSettings.getSkippableTests(relativeModulePath);
142+
break;
143+
case FLAKY:
144+
tests = moduleExecutionSettings.getFlakyTests(relativeModulePath);
145+
break;
146+
default:
147+
throw new IllegalArgumentException(
148+
"Unexpected test data type requested: " + testDataType);
149+
}
150+
151+
return new TestDataResponse(tests);
139152
} catch (Exception e) {
140153
return new ErrorResponse("Error while getting skippable tests: " + e.getMessage());
141154
}

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/DDTestFrameworkModule.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package datadog.trace.civisibility;
22

3-
import datadog.trace.api.civisibility.config.SkippableTest;
3+
import datadog.trace.api.civisibility.config.TestIdentifier;
4+
import datadog.trace.api.civisibility.retry.TestRetryPolicy;
45
import javax.annotation.Nullable;
56

67
/** Test module abstraction that is used by test framework instrumentations (e.g. JUnit, TestNG) */
@@ -17,7 +18,7 @@ DDTestSuiteImpl testSuiteStart(
1718
* @param test Test to be checked
1819
* @return {@code true} if the test can be skipped, {@code false} otherwise
1920
*/
20-
boolean isSkippable(SkippableTest test);
21+
boolean isSkippable(TestIdentifier test);
2122

2223
/**
2324
* Checks if a given test can be skipped with Intelligent Test Runner or not. It the test is
@@ -26,7 +27,9 @@ DDTestSuiteImpl testSuiteStart(
2627
* @param test Test to be checked
2728
* @return {@code true} if the test can be skipped, {@code false} otherwise
2829
*/
29-
boolean skip(SkippableTest test);
30+
boolean skip(TestIdentifier test);
31+
32+
TestRetryPolicy retryPolicy(TestIdentifier test);
3033

3134
void end(Long startTime);
3235
}

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/DDTestFrameworkModuleImpl.java‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,19 @@
33
import datadog.trace.api.Config;
44
import datadog.trace.api.DDTags;
55
import datadog.trace.api.civisibility.config.ModuleExecutionSettings;
6-
import datadog.trace.api.civisibility.config.SkippableTest;
6+
import datadog.trace.api.civisibility.config.TestIdentifier;
7+
import datadog.trace.api.civisibility.retry.TestRetryPolicy;
78
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
89
import datadog.trace.bootstrap.instrumentation.api.Tags;
910
import datadog.trace.civisibility.codeowners.Codeowners;
1011
import datadog.trace.civisibility.config.JvmInfo;
1112
import datadog.trace.civisibility.config.ModuleExecutionSettingsFactory;
1213
import datadog.trace.civisibility.coverage.CoverageProbeStoreFactory;
1314
import datadog.trace.civisibility.decorator.TestDecorator;
15+
import datadog.trace.civisibility.retry.NeverRetry;
16+
import datadog.trace.civisibility.retry.RetryIfFailed;
1417
import datadog.trace.civisibility.source.MethodLinesResolver;
1518
import datadog.trace.civisibility.source.SourcePathResolver;
16-
import java.util.ArrayList;
1719
import java.util.Collection;
1820
import java.util.HashSet;
1921
import java.util.concurrent.atomic.LongAdder;
@@ -28,7 +30,8 @@
2830
public class DDTestFrameworkModuleImpl extends DDTestModuleImpl implements DDTestFrameworkModule {
2931

3032
private final LongAdder testsSkipped = new LongAdder();
31-
private final Collection<SkippableTest> skippableTests;
33+
private final Collection<TestIdentifier> skippableTests;
34+
private final Collection<TestIdentifier> flakyTests;
3235
private final boolean codeCoverageEnabled;
3336
private final boolean itrEnabled;
3437

@@ -62,21 +65,17 @@ public DDTestFrameworkModuleImpl(
6265
moduleExecutionSettingsFactory.create(JvmInfo.CURRENT_JVM, moduleName);
6366
codeCoverageEnabled = moduleExecutionSettings.isCodeCoverageEnabled();
6467
itrEnabled = moduleExecutionSettings.isItrEnabled();
65-
Collection<SkippableTest> moduleSkippableTests =
66-
moduleExecutionSettings.getSkippableTests(moduleName);
67-
skippableTests =
68-
moduleSkippableTests.size() > 100
69-
? new HashSet<>(moduleSkippableTests)
70-
: new ArrayList<>(moduleSkippableTests);
68+
skippableTests = new HashSet<>(moduleExecutionSettings.getSkippableTests(moduleName));
69+
flakyTests = new HashSet<>(moduleExecutionSettings.getFlakyTests(moduleName));
7170
}
7271

7372
@Override
74-
public boolean isSkippable(SkippableTest test) {
73+
public boolean isSkippable(TestIdentifier test) {
7574
return test != null && skippableTests.contains(test);
7675
}
7776

7877
@Override
79-
public boolean skip(SkippableTest test) {
78+
public boolean skip(TestIdentifier test) {
8079
if (isSkippable(test)) {
8180
testsSkipped.increment();
8281
return true;
@@ -85,6 +84,13 @@ public boolean skip(SkippableTest test) {
8584
}
8685
}
8786

87+
@Override
88+
public TestRetryPolicy retryPolicy(TestIdentifier test) {
89+
return test != null && flakyTests.contains(test)
90+
? new RetryIfFailed(config.getCiVisibilityFlakyRetryCount())
91+
: NeverRetry.INSTANCE;
92+
}
93+
8894
@Override
8995
public void end(@Nullable Long endTime) {
9096
if (codeCoverageEnabled) {

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/DDTestFrameworkModuleProxy.java‎

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
package datadog.trace.civisibility;
22

33
import datadog.trace.api.Config;
4-
import datadog.trace.api.civisibility.config.SkippableTest;
4+
import datadog.trace.api.civisibility.config.TestIdentifier;
55
import datadog.trace.api.civisibility.coverage.CoverageDataSupplier;
6+
import datadog.trace.api.civisibility.retry.TestRetryPolicy;
67
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
78
import datadog.trace.bootstrap.instrumentation.api.Tags;
89
import datadog.trace.civisibility.codeowners.Codeowners;
@@ -11,13 +12,15 @@
1112
import datadog.trace.civisibility.decorator.TestDecorator;
1213
import datadog.trace.civisibility.ipc.ModuleExecutionResult;
1314
import datadog.trace.civisibility.ipc.SignalClient;
14-
import datadog.trace.civisibility.ipc.SkippableTestsRequest;
15-
import datadog.trace.civisibility.ipc.SkippableTestsResponse;
15+
import datadog.trace.civisibility.ipc.TestDataRequest;
16+
import datadog.trace.civisibility.ipc.TestDataResponse;
17+
import datadog.trace.civisibility.ipc.TestDataType;
1618
import datadog.trace.civisibility.ipc.TestFramework;
19+
import datadog.trace.civisibility.retry.NeverRetry;
20+
import datadog.trace.civisibility.retry.RetryIfFailed;
1721
import datadog.trace.civisibility.source.MethodLinesResolver;
1822
import datadog.trace.civisibility.source.SourcePathResolver;
1923
import java.net.InetSocketAddress;
20-
import java.util.ArrayList;
2124
import java.util.Collection;
2225
import java.util.Collections;
2326
import java.util.HashSet;
@@ -49,7 +52,8 @@ public class DDTestFrameworkModuleProxy implements DDTestFrameworkModule {
4952
private final MethodLinesResolver methodLinesResolver;
5053
private final CoverageProbeStoreFactory coverageProbeStoreFactory;
5154
private final LongAdder testsSkipped = new LongAdder();
52-
private final Collection<SkippableTest> skippableTests;
55+
private final Collection<TestIdentifier> skippableTests;
56+
private final Collection<TestIdentifier> flakyTests;
5357
private final Collection<TestFramework> testFrameworks = ConcurrentHashMap.newKeySet();
5458

5559
public DDTestFrameworkModuleProxy(
@@ -76,35 +80,44 @@ public DDTestFrameworkModuleProxy(
7680
this.methodLinesResolver = methodLinesResolver;
7781
this.coverageProbeStoreFactory = coverageProbeStoreFactory;
7882
this.skippableTests = fetchSkippableTests(moduleName, signalServerAddress);
83+
this.flakyTests = fetchFlakyTests(moduleName, signalServerAddress);
7984
}
8085

81-
private Collection<SkippableTest> fetchSkippableTests(
86+
private Collection<TestIdentifier> fetchSkippableTests(
8287
String moduleName, InetSocketAddress signalServerAddress) {
83-
if (!config.isCiVisibilityItrEnabled()) {
84-
return Collections.emptyList();
85-
}
88+
return config.isCiVisibilityItrEnabled()
89+
? fetchTestData(TestDataType.SKIPPABLE, moduleName, signalServerAddress)
90+
: Collections.emptyList();
91+
}
92+
93+
private Collection<TestIdentifier> fetchFlakyTests(
94+
String moduleName, InetSocketAddress signalServerAddress) {
95+
return config.isCiVisibilityFlakyRetryEnabled()
96+
? fetchTestData(TestDataType.FLAKY, moduleName, signalServerAddress)
97+
: Collections.emptyList();
98+
}
8699

87-
SkippableTestsRequest request = new SkippableTestsRequest(moduleName, JvmInfo.CURRENT_JVM);
100+
private Collection<TestIdentifier> fetchTestData(
101+
TestDataType testDataType, String moduleName, InetSocketAddress signalServerAddress) {
102+
TestDataRequest request = new TestDataRequest(testDataType, moduleName, JvmInfo.CURRENT_JVM);
88103
try (SignalClient signalClient = new SignalClient(signalServerAddress)) {
89-
SkippableTestsResponse response = (SkippableTestsResponse) signalClient.send(request);
90-
Collection<SkippableTest> moduleSkippableTests = response.getTests();
91-
log.debug("Received {} skippable tests", moduleSkippableTests.size());
92-
return moduleSkippableTests.size() > 100
93-
? new HashSet<>(moduleSkippableTests)
94-
: new ArrayList<>(moduleSkippableTests);
104+
TestDataResponse response = (TestDataResponse) signalClient.send(request);
105+
Collection<TestIdentifier> testData = response.getTests();
106+
log.debug("Received {} {} tests", testData.size(), testDataType);
107+
return new HashSet<>(testData);
95108
} catch (Exception e) {
96-
log.error("Error while requesting skippable tests", e);
109+
log.error("Error while requesting {} test data", testDataType, e);
97110
return Collections.emptySet();
98111
}
99112
}
100113

101114
@Override
102-
public boolean isSkippable(SkippableTest test) {
115+
public boolean isSkippable(TestIdentifier test) {
103116
return test != null && skippableTests.contains(test);
104117
}
105118

106119
@Override
107-
public boolean skip(SkippableTest test) {
120+
public boolean skip(TestIdentifier test) {
108121
if (isSkippable(test)) {
109122
testsSkipped.increment();
110123
return true;
@@ -113,6 +126,13 @@ public boolean skip(SkippableTest test) {
113126
}
114127
}
115128

129+
@Override
130+
public TestRetryPolicy retryPolicy(TestIdentifier test) {
131+
return test != null && flakyTests.contains(test)
132+
? new RetryIfFailed(config.getCiVisibilityFlakyRetryCount())
133+
: NeverRetry.INSTANCE;
134+
}
135+
116136
@Override
117137
public void end(@Nullable Long endTime) {
118138
// we have no span locally,

0 commit comments

Comments
 (0)