Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,6 @@ public List<ExecutionsByDuration> getExecutionsByDuration() {
return executionsByDuration;
}

public int getExecutions(long durationMillis) {
for (ExecutionsByDuration e : executionsByDuration) {
if (durationMillis <= e.durationMillis) {
return e.executions;
}
}
return 0;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -61,39 +52,4 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(enabled, executionsByDuration, faultySessionThreshold);
}

public static final class ExecutionsByDuration {
public final long durationMillis;
public final int executions;

public ExecutionsByDuration(long durationMillis, int executions) {
this.durationMillis = durationMillis;
this.executions = executions;
}

public long getDurationMillis() {
return durationMillis;
}

public int getExecutions() {
return executions;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
ExecutionsByDuration that = (ExecutionsByDuration) o;
return durationMillis == that.durationMillis && executions == that.executions;
}

@Override
public int hashCode() {
return Objects.hash(durationMillis, executions);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,14 @@ public EarlyFlakeDetectionSettings fromJson(Map<String, Object> json) {
Boolean enabled = (Boolean) json.get("enabled");
Double faultySessionThreshold = (Double) json.get("faulty_session_threshold");

List<EarlyFlakeDetectionSettings.ExecutionsByDuration> executionsByDuration;
List<ExecutionsByDuration> executionsByDuration;
Map<String, Double> slowTestRetries = (Map<String, Double>) json.get("slow_test_retries");
if (slowTestRetries != null) {
executionsByDuration = new ArrayList<>(slowTestRetries.size());
for (Map.Entry<String, Double> e : slowTestRetries.entrySet()) {
long durationMillis = parseDuration(e.getKey());
int retries = e.getValue().intValue();
executionsByDuration.add(
new EarlyFlakeDetectionSettings.ExecutionsByDuration(durationMillis, retries));
executionsByDuration.add(new ExecutionsByDuration(durationMillis, retries));
}
executionsByDuration.sort(Comparator.comparingLong(r -> r.durationMillis));
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public static void serialize(Serializer serializer, EarlyFlakeDetectionSettings
serializer.write(settings.getFaultySessionThreshold());
serializer.write(
settings.getExecutionsByDuration(),
EarlyFlakeDetectionSettingsSerializer::serializeExecutionsByDuration);
ExecutionsByDuration.ExecutionsByDurationSerializer::serialize);
}

public static EarlyFlakeDetectionSettings deserialize(ByteBuffer buf) {
Expand All @@ -25,22 +25,8 @@ public static EarlyFlakeDetectionSettings deserialize(ByteBuffer buf) {
}

int faultySessionThreshold = Serializer.readInt(buf);
List<EarlyFlakeDetectionSettings.ExecutionsByDuration> executionsByDuration =
Serializer.readList(
buf, EarlyFlakeDetectionSettingsSerializer::deserializeExecutionsByDuration);
List<ExecutionsByDuration> executionsByDuration =
Serializer.readList(buf, ExecutionsByDuration.ExecutionsByDurationSerializer::deserialize);
return new EarlyFlakeDetectionSettings(enabled, executionsByDuration, faultySessionThreshold);
}

private static void serializeExecutionsByDuration(
Serializer serializer,
EarlyFlakeDetectionSettings.ExecutionsByDuration executionsByDuration) {
serializer.write(executionsByDuration.durationMillis);
serializer.write(executionsByDuration.executions);
}

private static EarlyFlakeDetectionSettings.ExecutionsByDuration deserializeExecutionsByDuration(
ByteBuffer buf) {
return new EarlyFlakeDetectionSettings.ExecutionsByDuration(
Serializer.readLong(buf), Serializer.readInt(buf));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ public static ByteBuffer serialize(ExecutionSettings settings) {

s.write(settings.testSettings, TestFQNSerializer::serialize, Serializer::write);
s.write(
settings.settingsCount, TestSetting.TestSettingsSerializer::serialize, Serializer::write);
settings.settingsCount, TestSetting.TestSettingSerializer::serialize, Serializer::write);

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

Expand Down Expand Up @@ -345,7 +345,7 @@ public static ExecutionSettings deserialize(ByteBuffer buffer) {
Serializer.readMap(
buffer,
() -> new EnumMap<>(TestSetting.class),
TestSetting.TestSettingsSerializer::deserialize,
TestSetting.TestSettingSerializer::deserialize,
Serializer::readInt);

Diff diff = Diff.SERIALIZER.deserialize(buffer);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package datadog.trace.civisibility.config;

import datadog.trace.civisibility.ipc.serialization.Serializer;
import java.nio.ByteBuffer;
import java.util.Objects;

public final class ExecutionsByDuration {
public final long durationMillis;
public final int executions;

public ExecutionsByDuration(long durationMillis, int executions) {
this.durationMillis = durationMillis;
this.executions = executions;
}

public long getDurationMillis() {
return durationMillis;
}

public int getExecutions() {
return executions;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
ExecutionsByDuration that = (ExecutionsByDuration) o;
return durationMillis == that.durationMillis && executions == that.executions;
}

@Override
public int hashCode() {
return Objects.hash(durationMillis, executions);
}

public static class ExecutionsByDurationSerializer {
public static void serialize(Serializer serializer, ExecutionsByDuration executionsByDuration) {
serializer.write(executionsByDuration.durationMillis);
serializer.write(executionsByDuration.executions);
}

public static ExecutionsByDuration deserialize(ByteBuffer buf) {
return new ExecutionsByDuration(Serializer.readLong(buf), Serializer.readInt(buf));
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package datadog.trace.civisibility.config;

import java.util.Collections;
import java.util.List;
import java.util.Objects;

public class TestManagementSettings {
Expand All @@ -22,6 +24,10 @@ public int getAttemptToFixRetries() {
return attemptToFixRetries;
}

public List<ExecutionsByDuration> getExecutionsByDuration() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this to something like getAttemptToFixExecutions to emphasise that this is relevant only for attempt to fix tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, optionally, to promote encapsulation, we could make TestManagementSettingsSerializer a nested class in this one, and remove the getAttemptToFixRetries method

return Collections.singletonList(new ExecutionsByDuration(Long.MAX_VALUE, attemptToFixRetries));
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public static boolean isSet(int mask, TestSetting setting) {
return (mask & setting.flag) != 0;
}

public static class TestSettingsSerializer {
public static class TestSettingSerializer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we refer to this class as TestSetting.TestSettingSerializer, should we rename it for brevity? TestSetting.Serializer looks nicer to me.
Same goes for ExecutionsByDuration.ExecutionsByDurationSerializer

public static void serialize(Serializer serializer, TestSetting setting) {
serializer.write(setting.flag);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ TestSuiteImpl testSuiteStart(

boolean isDisabled(TestIdentifier test);

boolean isAttemptToFix(TestIdentifier test);

/**
* Returns the reason for skipping a test, IF it can be skipped.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import datadog.trace.api.civisibility.telemetry.tag.BrowserDriver;
import datadog.trace.api.civisibility.telemetry.tag.EventType;
import datadog.trace.api.civisibility.telemetry.tag.HasFailedAllRetries;
import datadog.trace.api.civisibility.telemetry.tag.IsAttemptToFix;
import datadog.trace.api.civisibility.telemetry.tag.IsDisabled;
import datadog.trace.api.civisibility.telemetry.tag.IsModified;
import datadog.trace.api.civisibility.telemetry.tag.IsNew;
Expand Down Expand Up @@ -280,6 +281,9 @@ public void end(@Nullable Long endTime) {
span.getTag(Tags.TEST_IS_MODIFIED) != null ? IsModified.TRUE : null,
span.getTag(Tags.TEST_TEST_MANAGEMENT_IS_QUARANTINED) != null ? IsQuarantined.TRUE : null,
span.getTag(Tags.TEST_TEST_MANAGEMENT_IS_TEST_DISABLED) != null ? IsDisabled.TRUE : null,
span.getTag(Tags.TEST_TEST_MANAGEMENT_IS_ATTEMPT_TO_FIX) != null
? IsAttemptToFix.TRUE
: null,
span.getTag(Tags.TEST_IS_RETRY) != null ? IsRetry.TRUE : null,
span.getTag(Tags.TEST_HAS_FAILED_ALL_RETRIES) != null ? HasFailedAllRetries.TRUE : null,
retryReason instanceof TagValue ? (TagValue) retryReason : null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ public boolean isDisabled(TestIdentifier test) {
return executionStrategy.isDisabled(test);
}

@Override
public boolean isAttemptToFix(TestIdentifier test) {
return executionStrategy.isAttemptToFix(test);
}

@Nullable
@Override
public SkipReason skipReason(TestIdentifier test) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ public boolean isDisabled(TestIdentifier test) {
return executionStrategy.isDisabled(test);
}

@Override
public boolean isAttemptToFix(TestIdentifier test) {
return executionStrategy.isAttemptToFix(test);
}

@Nullable
@Override
public SkipReason skipReason(TestIdentifier test) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@ public void onTestStart(
test.setTag(Tags.TEST_TEST_MANAGEMENT_IS_TEST_DISABLED, true);
}

if (testModule.isAttemptToFix(thisTest)) {
test.setTag(Tags.TEST_TEST_MANAGEMENT_IS_ATTEMPT_TO_FIX, true);
}

if (testExecutionHistory != null) {
RetryReason retryReason = testExecutionHistory.currentExecutionRetryReason();
if (retryReason != null) {
Expand Down Expand Up @@ -249,9 +253,14 @@ public void onTestFinish(
return;
}

TestIdentifier thisTest = test.getIdentifier();
if (testExecutionHistory != null) {
if (test.hasFailed() && testExecutionHistory.hasFailedAllRetries()) {
test.setTag(Tags.TEST_HAS_FAILED_ALL_RETRIES, true);
} else if (!test.hasFailed()
&& testModule.isAttemptToFix(thisTest)
&& testExecutionHistory.hasSucceededAllRetries()) {
test.setTag(Tags.TEST_TEST_MANAGEMENT_ATTEMPT_TO_FIX_PASSED, true);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,9 @@ public RetryReason currentExecutionRetryReason() {
public boolean hasFailedAllRetries() {
return false;
}

@Override
public boolean hasSucceededAllRetries() {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,9 @@ private boolean currentExecutionIsRetry() {
public boolean hasFailedAllRetries() {
return currentExecutionIsLast() && !successfulExecutionSeen;
}

@Override
public boolean hasSucceededAllRetries() {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,29 @@

import datadog.trace.api.civisibility.execution.TestExecutionPolicy;
import datadog.trace.api.civisibility.telemetry.tag.RetryReason;
import datadog.trace.civisibility.config.EarlyFlakeDetectionSettings;
import datadog.trace.civisibility.config.ExecutionsByDuration;
import java.util.List;
import org.jetbrains.annotations.Nullable;

/** Runs a test case N times (N depends on test duration) regardless of success or failure. */
public class RunNTimes implements TestExecutionPolicy {

private final EarlyFlakeDetectionSettings earlyFlakeDetectionSettings;
private final boolean suppressFailures;
private int executions;
private final List<ExecutionsByDuration> executionsByDuration;
private int maxExecutions;
private boolean successfulExecutionSeen;
private int successfulExecutionsSeen;
private final RetryReason retryReason;

public RunNTimes(
EarlyFlakeDetectionSettings earlyFlakeDetectionSettings, boolean suppressFailures) {
this.earlyFlakeDetectionSettings = earlyFlakeDetectionSettings;
List<ExecutionsByDuration> executionsByDuration,
boolean suppressFailures,
RetryReason retryReason) {
this.suppressFailures = suppressFailures;
this.executions = 0;
this.maxExecutions = earlyFlakeDetectionSettings.getExecutions(0);
this.executionsByDuration = executionsByDuration;
this.maxExecutions = getExecutions(0);
this.retryReason = retryReason;
}

@Override
Expand All @@ -36,19 +41,29 @@ public boolean suppressFailures() {
return suppressFailures;
}

private int getExecutions(long durationMillis) {
for (ExecutionsByDuration e : executionsByDuration) {
if (durationMillis <= e.getDurationMillis()) {
return e.getExecutions();
}
}
return 0;
}

@Override
public boolean retry(boolean successful, long durationMillis) {
successfulExecutionSeen |= successful;
// adjust maximum retries based on the now known test duration
int maxExecutionsForGivenDuration = earlyFlakeDetectionSettings.getExecutions(durationMillis);
if (successful) {
++successfulExecutionsSeen;
}
int maxExecutionsForGivenDuration = getExecutions(durationMillis);
maxExecutions = Math.min(maxExecutions, maxExecutionsForGivenDuration);
return ++executions < maxExecutions;
}

@Nullable
@Override
public RetryReason currentExecutionRetryReason() {
return currentExecutionIsRetry() ? RetryReason.efd : null;
return currentExecutionIsRetry() ? retryReason : null;
}

private boolean currentExecutionIsRetry() {
Expand All @@ -57,6 +72,11 @@ private boolean currentExecutionIsRetry() {

@Override
public boolean hasFailedAllRetries() {
return currentExecutionIsLast() && !successfulExecutionSeen;
return currentExecutionIsLast() && successfulExecutionsSeen == 0;
}

@Override
public boolean hasSucceededAllRetries() {
return currentExecutionIsLast() && successfulExecutionsSeen == maxExecutions - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think successfulExecutionsSeen == maxExecutions - 1 will work in all possible cases.

Imagine we have an initial value of maxExecutions as 10.
We run the test case 4 or 5 times, all of the executions are successful, but then the last one takes a while to complete (for some arbitrary reason, e.g. the network is very slow all of a sudden).
We re-evaluate max executions based on the last duration, and now the value is 2.
Given that we have already ran the test 4 times, successfulExecutionsSeen == maxExecutions - 1 is never going to be true, even if all the executions really are successful.

Yes, for now this method is only relevant for "attempted to fix" test cases, and for now the number of executions for such tests is constant. But having these "hidden assumptions" in our code makes it fragile: if things change a year from now we might not remember that there's an invariant that no longer holds.

I'd add a totalExecutionsSeen field and compare successfulExecutionsSeen with it (there are, of course, many other ways of implementing this: two boolean flags, list of execution statuses, etc - choose whichever you like the most).

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,9 @@ public RetryReason currentExecutionRetryReason() {
public boolean hasFailedAllRetries() {
return false;
}

@Override
public boolean hasSucceededAllRetries() {
return false;
}
}
Loading
Loading