Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request Test Management tests list #8345

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Request Test Management tests list #8345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...agent-ci-visibility/src/main/java/datadog/trace/civisibility/config/ConfigurationApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ public Map<String, Collection<TestIdentifier>> getKnownTestsByModule(
return Collections.emptyMap();
}

@Override
public Map<String, Map<String, Collection<TestIdentifier>>> getTestManagementTestsByModule(
TracerEnvironment tracerEnvironment) {
return Collections.emptyMap();
}

@Override
public ChangedFiles getChangedFiles(TracerEnvironment tracerEnvironment) {
return ChangedFiles.EMPTY;
Expand All @@ -50,5 +56,8 @@ Map<String, Collection<TestIdentifier>> getFlakyTestsByModule(TracerEnvironment
Map<String, Collection<TestIdentifier>> getKnownTestsByModule(TracerEnvironment tracerEnvironment)
throws IOException;

Map<String, Map<String, Collection<TestIdentifier>>> getTestManagementTestsByModule(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map<String, Map<String, Collection<TestIdentifier>>> might be difficult to comprehend when we revisit this code in a few months :)

Let's see if we can apply stronger typing, e.g. something along the lines of Map<String, Map<TestManagementStatus, Collection<TestIdentifier>>>.
Where TestManagementStatus is an enum with possible values quarantined, disabled, attemptToFix (TestManagementStatus might not be the best name for it, we can consider alternatives too, like TestManagementStrategy or TestManagementBehavior).

My main point is what we expect here is a set of values that are known to the tracer, if we cement this with some compile-time safety, the code will be easier to reason about.

TracerEnvironment tracerEnvironment) throws IOException;

ChangedFiles getChangedFiles(TracerEnvironment tracerEnvironment) throws IOException;
}
147 changes: 147 additions & 0 deletions ...t-ci-visibility/src/main/java/datadog/trace/civisibility/config/ConfigurationApiImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public class ConfigurationApiImpl implements ConfigurationApi {
private static final String CHANGED_FILES_URI = "ci/tests/diffs";
private static final String FLAKY_TESTS_URI = "ci/libraries/tests/flaky";
private static final String KNOWN_TESTS_URI = "ci/libraries/tests";
private static final String TEST_MANAGEMENT_TESTS_URI = "test/libraries/test-management/tests";

private final BackendApi backendApi;
private final CiVisibilityMetricCollector metricCollector;
Expand All @@ -63,6 +64,7 @@ public class ConfigurationApiImpl implements ConfigurationApi {
private final JsonAdapter<EnvelopeDto<CiVisibilitySettings>> settingsResponseAdapter;
private final JsonAdapter<MultiEnvelopeDto<TestIdentifierJson>> testIdentifiersResponseAdapter;
private final JsonAdapter<EnvelopeDto<KnownTestsDto>> testFullNamesResponseAdapter;
private final JsonAdapter<EnvelopeDto<TestManagementTestsDto>> testManagementTestsResponseAdapter;
private final JsonAdapter<EnvelopeDto<ChangedFiles>> changedFilesResponseAdapter;

public ConfigurationApiImpl(BackendApi backendApi, CiVisibilityMetricCollector metricCollector) {
Expand Down Expand Up @@ -105,6 +107,11 @@ public ConfigurationApiImpl(BackendApi backendApi, CiVisibilityMetricCollector m
ConfigurationApiImpl.class, EnvelopeDto.class, KnownTestsDto.class);
testFullNamesResponseAdapter = moshi.adapter(testFullNamesResponseType);

ParameterizedType testManagementTestsResponseType =
Types.newParameterizedTypeWithOwner(
ConfigurationApiImpl.class, EnvelopeDto.class, TestManagementTestsDto.class);
testManagementTestsResponseAdapter = moshi.adapter(testManagementTestsResponseType);

ParameterizedType changedFilesResponseAdapterType =
Types.newParameterizedTypeWithOwner(
ConfigurationApiImpl.class, EnvelopeDto.class, ChangedFiles.class);
Expand Down Expand Up @@ -309,6 +316,90 @@ private Map<String, Collection<TestIdentifier>> parseTestIdentifiers(KnownTestsD
: null;
}

@Override
public Map<String, Map<String, Collection<TestIdentifier>>> getTestManagementTestsByModule(
TracerEnvironment tracerEnvironment) throws IOException {
OkHttpUtils.CustomListener telemetryListener =
new TelemetryListener.Builder(metricCollector)
.requestCount(CiVisibilityCountMetric.TEST_MANAGEMENT_TESTS_DETECTION_REQUEST)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "detection" made sense in "Impacted Tests Detection" as that was the name of the feature.
Here, I think, TEST_MANAGEMENT_TESTS_REQUEST is enough

.requestErrors(CiVisibilityCountMetric.TEST_MANAGEMENT_TESTS_DETECTION_REQUEST_ERRORS)
.requestDuration(CiVisibilityDistributionMetric.TEST_MANAGEMENT_TESTS_REQUEST_MS)
.responseBytes(CiVisibilityDistributionMetric.TEST_MANAGEMENT_TESTS_RESPONSE_BYTES)
.build();

String uuid = uuidGenerator.get();
EnvelopeDto<TracerEnvironment> request =
new EnvelopeDto<>(new DataDto<>(uuid, "ci_app_libraries_tests_request", tracerEnvironment));
String json = requestAdapter.toJson(request);
RequestBody requestBody = RequestBody.create(JSON, json);
TestManagementTestsDto testManagementTestsDto =
backendApi.post(
TEST_MANAGEMENT_TESTS_URI,
requestBody,
is ->
testManagementTestsResponseAdapter.fromJson(Okio.buffer(Okio.source(is)))
.data
.attributes,
telemetryListener,
false);

return parseTestManagementTests(testManagementTestsDto);
}

private Map<String, Map<String, Collection<TestIdentifier>>> parseTestManagementTests(
TestManagementTestsDto testsManagementTestsDto) {
int testManagementTestsCount = 0;

Map<String, Collection<TestIdentifier>> quarantinedTestsByModule = new HashMap<>();
Map<String, Collection<TestIdentifier>> disabledTestsByModule = new HashMap<>();
Map<String, Collection<TestIdentifier>> attemptToFixTestsByModule = new HashMap<>();

for (Map.Entry<String, TestManagementTestsDto.Suites> e :
testsManagementTestsDto.getModules().entrySet()) {
String moduleName = e.getKey();
Map<String, TestManagementTestsDto.Tests> testsBySuiteName = e.getValue().getSuites();

for (Map.Entry<String, TestManagementTestsDto.Tests> se : testsBySuiteName.entrySet()) {
String suiteName = se.getKey();
Map<String, TestManagementTestsDto.Properties> tests = se.getValue().getTests();

testManagementTestsCount += tests.size();

for (Map.Entry<String, TestManagementTestsDto.Properties> te : tests.entrySet()) {
String testName = te.getKey();
TestManagementTestsDto.Properties properties = te.getValue();
if (properties.isQuarantined()) {
quarantinedTestsByModule
.computeIfAbsent(moduleName, k -> new HashSet<>())
.add(new TestIdentifier(suiteName, testName, null));
}
if (properties.isDisabled()) {
disabledTestsByModule
.computeIfAbsent(moduleName, k -> new HashSet<>())
.add(new TestIdentifier(suiteName, testName, null));
}
if (properties.isAttemptToFix()) {
attemptToFixTestsByModule
.computeIfAbsent(moduleName, k -> new HashSet<>())
.add(new TestIdentifier(suiteName, testName, null));
}
}
}
}

Map<String, Map<String, Collection<TestIdentifier>>> testsByTypeByModule = new HashMap<>();
testsByTypeByModule.put("quarantined", quarantinedTestsByModule);
testsByTypeByModule.put("disabled", disabledTestsByModule);
testsByTypeByModule.put("attempt_to_fix", attemptToFixTestsByModule);

LOGGER.debug("Received {} test management tests in total", testManagementTestsCount);
metricCollector.add(
CiVisibilityDistributionMetric.TEST_MANAGEMENT_TESTS_RESPONSE_TESTS,
testManagementTestsCount);

return testsByTypeByModule;
}

@Override
public ChangedFiles getChangedFiles(TracerEnvironment tracerEnvironment) throws IOException {
OkHttpUtils.CustomListener telemetryListener =
Expand Down Expand Up @@ -427,4 +518,60 @@ private KnownTestsDto(Map<String, Map<String, List<String>>> tests) {
this.tests = tests;
}
}

private static final class TestManagementTestsDto {
private static final class Properties {
private final Map<String, Boolean> properties;

private Properties(Map<String, Boolean> properties) {
this.properties = properties;
}

public Boolean isQuarantined() {
return properties != null ? properties.getOrDefault("quarantined", false) : false;
}

public Boolean isDisabled() {
return properties != null ? properties.getOrDefault("disabled", false) : false;
}

public Boolean isAttemptToFix() {
return properties != null ? properties.getOrDefault("attempt_to_fix", false) : false;
}
}

private static final class Tests {
private final Map<String, Properties> tests;

private Tests(Map<String, Properties> tests) {
this.tests = tests;
}

public Map<String, Properties> getTests() {
return tests != null ? tests : Collections.emptyMap();
}
}

private static final class Suites {
private final Map<String, Tests> suites;

private Suites(Map<String, Tests> suites) {
this.suites = suites;
}

public Map<String, Tests> getSuites() {
return suites != null ? suites : Collections.emptyMap();
}
}

private final Map<String, Suites> modules;

private TestManagementTestsDto(Map<String, Suites> modules) {
this.modules = modules;
}

public Map<String, Suites> getModules() {
return modules != null ? modules : Collections.emptyMap();
}
}
}
60 changes: 45 additions & 15 deletions ...gent-ci-visibility/src/main/java/datadog/trace/civisibility/config/ExecutionSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@ public class ExecutionSettings {
null,
Collections.emptyMap(),
Collections.emptyMap(),
Collections.emptyList(),
null,
null,
Collections.emptyList(),
Collections.emptyList(),
Collections.emptyList(),
LineDiff.EMPTY);

private final boolean itrEnabled;
Expand All @@ -44,9 +46,11 @@ public class ExecutionSettings {
@Nullable private final String itrCorrelationId;
@Nonnull private final Map<TestIdentifier, TestMetadata> skippableTests;
@Nonnull private final Map<String, BitSet> skippableTestsCoverage;
@Nonnull private final Collection<TestIdentifier> quarantinedTests;
@Nullable private final Collection<TestIdentifier> flakyTests;
@Nullable private final Collection<TestIdentifier> knownTests;
@Nonnull private final Collection<TestIdentifier> quarantinedTests;
@Nonnull private final Collection<TestIdentifier> disabledTests;
@Nonnull private final Collection<TestIdentifier> attemptToFixTests;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats' a lot of collections, I'm concerned about the possible memory footprint.
I think it'd be more efficient (and concise and easier to comprehend too) to have a single Map<TestIdentifer, TestSettings> where the value would contain all the possible settings for a given test (is flaky, is known, is quarantined, is disabled, is attempted to fix).
Could even be a map of <TestIdentifier, Integer> where integer is a bit mask (a bit low level, but memory-efficient, and the complexity is tolerable if we encapsulate it in this class and expose a higher-level API).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skippableTests should be separate though, as there the identifier includes test parameters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which makes me think of another issue: in some places where TestIdentifier is used, it includes parameters, while in others the parameters are omitted (because the backend doesn't give us that data).
It's not good that we have to guess whether a particular lookup should be done with parameters or without them, and the more logic we have using TestIdentifier, the more convoluted things become.

Perhaps it is the right time to introduce something like TestFQN (meaning "fully-qualified name"; as usual, naming things is the hard part) - something that identifies a test not taking its parameters into account - to make it clear what is expected where.

And then we could have something like TestIdentifer#toFQN to convert one thing into the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should flakyTests also be separate? I see that they can include test parameters as well in the request

Copy link
Contributor Author

@daniel-mohedano daniel-mohedano Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but it seems like they are never used with the parameters (the comparison is always done with TestIdentifier#withoutParameters)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think the backend returns parameters for flaky tests. The test case is not accurate

@Nonnull private final Diff pullRequestDiff;

public ExecutionSettings(
Expand All @@ -60,9 +64,11 @@ public ExecutionSettings(
@Nullable String itrCorrelationId,
@Nonnull Map<TestIdentifier, TestMetadata> skippableTests,
@Nonnull Map<String, BitSet> skippableTestsCoverage,
@Nonnull Collection<TestIdentifier> quarantinedTests,
@Nullable Collection<TestIdentifier> flakyTests,
@Nullable Collection<TestIdentifier> knownTests,
@Nonnull Collection<TestIdentifier> quarantinedTests,
@Nonnull Collection<TestIdentifier> disabledTests,
@Nonnull Collection<TestIdentifier> attemptToFixTests,
@Nonnull Diff pullRequestDiff) {
this.itrEnabled = itrEnabled;
this.codeCoverageEnabled = codeCoverageEnabled;
Expand All @@ -74,9 +80,11 @@ public ExecutionSettings(
this.itrCorrelationId = itrCorrelationId;
this.skippableTests = skippableTests;
this.skippableTestsCoverage = skippableTestsCoverage;
this.quarantinedTests = quarantinedTests;
this.flakyTests = flakyTests;
this.knownTests = knownTests;
this.quarantinedTests = quarantinedTests;
this.disabledTests = disabledTests;
this.attemptToFixTests = attemptToFixTests;
this.pullRequestDiff = pullRequestDiff;
}

Expand Down Expand Up @@ -130,11 +138,6 @@ public Map<TestIdentifier, TestMetadata> getSkippableTests() {
return skippableTests;
}

@Nonnull
public Collection<TestIdentifier> getQuarantinedTests() {
return quarantinedTests;
}

/**
* @return the list of known tests for the given module (can be empty), or {@code null} if known
* tests could not be obtained
Expand All @@ -153,6 +156,21 @@ public Collection<TestIdentifier> getFlakyTests() {
return flakyTests;
}

@Nonnull
public Collection<TestIdentifier> getQuarantinedTests() {
return quarantinedTests;
}

@Nonnull
public Collection<TestIdentifier> getDisabledTests() {
return disabledTests;
}

@Nonnull
public Collection<TestIdentifier> getAttemptToFixTests() {
return attemptToFixTests;
}

@Nonnull
public Diff getPullRequestDiff() {
return pullRequestDiff;
Expand All @@ -175,9 +193,11 @@ public boolean equals(Object o) {
&& Objects.equals(itrCorrelationId, that.itrCorrelationId)
&& Objects.equals(skippableTests, that.skippableTests)
&& Objects.equals(skippableTestsCoverage, that.skippableTestsCoverage)
&& Objects.equals(quarantinedTests, that.quarantinedTests)
&& Objects.equals(flakyTests, that.flakyTests)
&& Objects.equals(knownTests, that.knownTests)
&& Objects.equals(quarantinedTests, that.quarantinedTests)
&& Objects.equals(disabledTests, that.disabledTests)
&& Objects.equals(attemptToFixTests, that.attemptToFixTests)
&& Objects.equals(pullRequestDiff, that.pullRequestDiff);
}

Expand All @@ -192,9 +212,11 @@ public int hashCode() {
itrCorrelationId,
skippableTests,
skippableTestsCoverage,
quarantinedTests,
flakyTests,
knownTests,
quarantinedTests,
disabledTests,
attemptToFixTests,
pullRequestDiff);
}

Expand Down Expand Up @@ -231,9 +253,11 @@ public static ByteBuffer serialize(ExecutionSettings settings) {
TestMetadataSerializer::serialize);

s.write(settings.skippableTestsCoverage, Serializer::write, Serializer::write);
s.write(settings.quarantinedTests, TestIdentifierSerializer::serialize);
s.write(settings.flakyTests, TestIdentifierSerializer::serialize);
s.write(settings.knownTests, TestIdentifierSerializer::serialize);
s.write(settings.quarantinedTests, TestIdentifierSerializer::serialize);
s.write(settings.disabledTests, TestIdentifierSerializer::serialize);
s.write(settings.attemptToFixTests, TestIdentifierSerializer::serialize);

Diff.SERIALIZER.serialize(settings.pullRequestDiff, s);

Expand Down Expand Up @@ -262,12 +286,16 @@ public static ExecutionSettings deserialize(ByteBuffer buffer) {

Map<String, BitSet> skippableTestsCoverage =
Serializer.readMap(buffer, Serializer::readString, Serializer::readBitSet);
Collection<TestIdentifier> quarantinedTests =
Serializer.readSet(buffer, TestIdentifierSerializer::deserialize);
Collection<TestIdentifier> flakyTests =
Serializer.readSet(buffer, TestIdentifierSerializer::deserialize);
Collection<TestIdentifier> knownTests =
Serializer.readSet(buffer, TestIdentifierSerializer::deserialize);
Collection<TestIdentifier> quarantinedTests =
Serializer.readSet(buffer, TestIdentifierSerializer::deserialize);
Collection<TestIdentifier> disabledTests =
Serializer.readSet(buffer, TestIdentifierSerializer::deserialize);
Collection<TestIdentifier> attemptToFixTests =
Serializer.readSet(buffer, TestIdentifierSerializer::deserialize);

Diff diff = Diff.SERIALIZER.deserialize(buffer);

Expand All @@ -282,9 +310,11 @@ public static ExecutionSettings deserialize(ByteBuffer buffer) {
itrCorrelationId,
skippableTests,
skippableTestsCoverage,
quarantinedTests,
flakyTests,
knownTests,
quarantinedTests,
disabledTests,
attemptToFixTests,
diff);
}
}
Expand Down
Loading
Loading