Skip to content

Commit

Permalink
Error reporting improvements (#86)
Browse files Browse the repository at this point in the history 
* Define error codes for evaluation errors (EvaluationErrorCode) and expose them to the user (EvaluationDetails.ErrorCode)

* Define error codes for config data refresh errors (RefreshErrorCode) and expose them to the user (RefreshResult.ErrorCode)

* Introduce ConfigFetched hook

* Make subscription to hooks thread-safe

* Adjust tests
  • Loading branch information
@adams85
adams85 authored Apr 29, 2024
1 parent ed0a54d commit 241634c
Show file tree
Hide file tree
Showing 33 changed files with 593 additions and 140 deletions.
99 changes: 97 additions & 2 deletions src/ConfigCat.Client.Tests/ConfigCatClientTests.cs
View file

Large diffs are not rendered by default.

87 changes: 82 additions & 5 deletions src/ConfigCat.Client.Tests/ConfigServiceTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ public async Task LazyLoadConfigService_RefreshConfigAsync_ConfigChanged_ShouldR
var configChangedEvents = new ConcurrentQueue<ConfigChangedEventArgs>();
hooks.ConfigChanged += (s, e) => configChangedEvents.Enqueue(e);

var configFetchedEvents = new ConcurrentQueue<ConfigFetchedEventArgs>();
hooks.ConfigFetched += (s, e) => configFetchedEvents.Enqueue(e);

this.cacheMock
.Setup(m => m.GetAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
.ReturnsAsync(cachedPc);
Expand Down Expand Up @@ -196,6 +199,12 @@ public async Task LazyLoadConfigService_RefreshConfigAsync_ConfigChanged_ShouldR
Assert.IsTrue(configChangedEvents.TryDequeue(out var configChangedEvent));
Assert.AreSame(fetchedPc.Config, configChangedEvent.NewConfig);
Assert.AreEqual(0, configChangedEvents.Count);

Assert.AreEqual(1, configFetchedEvents.Count);
Assert.IsTrue(configFetchedEvents.TryDequeue(out var configFetchedEvent));
Assert.IsTrue(configFetchedEvent.IsInitiatedByUser);
Assert.IsTrue(configFetchedEvent.Result.IsSuccess);
Assert.AreEqual(RefreshErrorCode.None, configFetchedEvent.Result.ErrorCode);
}

[TestMethod]
Expand Down Expand Up @@ -416,6 +425,9 @@ public async Task ManualPollConfigService_GetConfigAsync_ShouldInvokeCacheGet()
var clientReadyEventCount = 0;
hooks.ClientReady += (s, e) => Interlocked.Increment(ref clientReadyEventCount);

var configFetchedEventCount = 0;
hooks.ConfigFetched += (s, e) => Interlocked.Increment(ref configFetchedEventCount);

this.cacheMock
.Setup(m => m.GetAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
.ReturnsAsync(cachedPc);
Expand All @@ -439,6 +451,8 @@ public async Task ManualPollConfigService_GetConfigAsync_ShouldInvokeCacheGet()
this.cacheMock.Verify(m => m.SetAsync(It.IsAny<string>(), It.IsAny<ProjectConfig>(), It.IsAny<CancellationToken>()), Times.Never);

Assert.AreEqual(1, Volatile.Read(ref clientReadyEventCount));

Assert.AreEqual(0, Volatile.Read(ref configFetchedEventCount));
}

[TestMethod]
Expand All @@ -455,6 +469,9 @@ public async Task ManualPollConfigService_RefreshConfigAsync_ShouldInvokeCacheGe
var clientReadyEventCount = 0;
hooks.ClientReady += (s, e) => Interlocked.Increment(ref clientReadyEventCount);

var configFetchedEvents = new ConcurrentQueue<ConfigFetchedEventArgs>();
hooks.ConfigFetched += (s, e) => configFetchedEvents.Enqueue(e);

byte callOrder = 1;

this.cacheMock
Expand Down Expand Up @@ -488,6 +505,12 @@ public async Task ManualPollConfigService_RefreshConfigAsync_ShouldInvokeCacheGe
this.cacheMock.Verify(m => m.SetAsync(It.IsAny<string>(), It.IsAny<ProjectConfig>(), It.IsAny<CancellationToken>()), Times.Once);

Assert.AreEqual(1, Volatile.Read(ref clientReadyEventCount));

Assert.AreEqual(1, configFetchedEvents.Count);
Assert.IsTrue(configFetchedEvents.TryDequeue(out var configFetchedEvent));
Assert.IsTrue(configFetchedEvent.IsInitiatedByUser);
Assert.IsTrue(configFetchedEvent.Result.IsSuccess);
Assert.AreEqual(RefreshErrorCode.None, configFetchedEvent.Result.ErrorCode);
}

[TestMethod]
Expand All @@ -504,6 +527,9 @@ public async Task ManualPollConfigService_RefreshConfigAsync_ConfigChanged_Shoul
var configChangedEvents = new ConcurrentQueue<ConfigChangedEventArgs>();
hooks.ConfigChanged += (s, e) => configChangedEvents.Enqueue(e);

var configFetchedEvents = new ConcurrentQueue<ConfigFetchedEventArgs>();
hooks.ConfigFetched += (s, e) => configFetchedEvents.Enqueue(e);

this.cacheMock
.Setup(m => m.GetAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
.ReturnsAsync(cachedPc);
Expand Down Expand Up @@ -531,6 +557,12 @@ public async Task ManualPollConfigService_RefreshConfigAsync_ConfigChanged_Shoul
Assert.IsTrue(configChangedEvents.TryDequeue(out var configChangedEvent));
Assert.AreSame(fetchedPc.Config, configChangedEvent.NewConfig);
Assert.AreEqual(0, configChangedEvents.Count);

Assert.AreEqual(1, configFetchedEvents.Count);
Assert.IsTrue(configFetchedEvents.TryDequeue(out var configFetchedEvent));
Assert.IsTrue(configFetchedEvent.IsInitiatedByUser);
Assert.IsTrue(configFetchedEvent.Result.IsSuccess);
Assert.AreEqual(RefreshErrorCode.None, configFetchedEvent.Result.ErrorCode);
}

[TestMethod]
Expand Down Expand Up @@ -624,6 +656,9 @@ public async Task AutoPollConfigService_GetConfig_ReturnsCachedConfigWhenCachedC
var clientReadyTcs = new TaskCompletionSource<object?>();
hooks.ClientReady += (s, e) => clientReadyTcs.TrySetResult(default);

var configFetchedEventCount = 0;
hooks.ConfigFetched += (s, e) => Interlocked.Increment(ref configFetchedEventCount);

var cache = new InMemoryConfigCache();
cache.Set(null!, cachedPc);

Expand Down Expand Up @@ -667,6 +702,8 @@ public async Task AutoPollConfigService_GetConfig_ReturnsCachedConfigWhenCachedC
{
Assert.IsTrue(clientReadyCalled);
}

Assert.AreEqual(0, Volatile.Read(ref configFetchedEventCount));
}

[DataRow(false)]
Expand All @@ -687,6 +724,9 @@ public async Task AutoPollConfigService_GetConfig_FetchesConfigWhenCachedConfigI
var clientReadyTcs = new TaskCompletionSource<object?>();
hooks.ClientReady += (s, e) => clientReadyTcs.TrySetResult(default);

var configFetchedEvents = new ConcurrentQueue<ConfigFetchedEventArgs>();
hooks.ConfigFetched += (s, e) => configFetchedEvents.Enqueue(e);

var cache = new InMemoryConfigCache();
cache.Set(null!, cachedPc);

Expand All @@ -710,9 +750,13 @@ public async Task AutoPollConfigService_GetConfig_FetchesConfigWhenCachedConfigI

// Allow some time for other initalization callbacks to execute.
using var cts = new CancellationTokenSource();
var task = await Task.WhenAny(clientReadyTcs.Task, Task.Delay(maxInitWaitTime, cts.Token));
var clientReadyTask = Task.Run(async () => await clientReadyTcs.Task);
var task = await Task.WhenAny(clientReadyTask, Task.Delay(maxInitWaitTime, cts.Token));
cts.Cancel();
clientReadyCalled = task == clientReadyTcs.Task && task.Status == TaskStatus.RanToCompletion;
clientReadyCalled = task == clientReadyTask && task.Status == TaskStatus.RanToCompletion;

// Wait for the hook event handlers to execute (as that might not happen if the service got disposed immediately).
SpinWait.SpinUntil(() => configFetchedEvents.TryPeek(out _), TimeSpan.FromSeconds(1));
}

// Assert
Expand All @@ -722,6 +766,12 @@ public async Task AutoPollConfigService_GetConfig_FetchesConfigWhenCachedConfigI
this.fetcherMock.Verify(m => m.FetchAsync(cachedPc, It.IsAny<CancellationToken>()), Times.Once);

Assert.IsTrue(clientReadyCalled);

Assert.AreEqual(1, configFetchedEvents.Count);
Assert.IsTrue(configFetchedEvents.TryDequeue(out var configFetchedEvent));
Assert.IsFalse(configFetchedEvent.IsInitiatedByUser);
Assert.IsTrue(configFetchedEvent.Result.IsSuccess);
Assert.AreEqual(RefreshErrorCode.None, configFetchedEvent.Result.ErrorCode);
}

[DataRow(false, false, true)]
Expand All @@ -746,11 +796,14 @@ public async Task AutoPollConfigService_GetConfig_ReturnsExpiredConfigWhenCantRe
var clientReadyTcs = new TaskCompletionSource<object?>();
hooks.ClientReady += (s, e) => clientReadyTcs.TrySetResult(default);

var configFetchedEvents = new ConcurrentQueue<ConfigFetchedEventArgs>();
hooks.ConfigFetched += (s, e) => configFetchedEvents.Enqueue(e);

var cache = new InMemoryConfigCache();
cache.Set(null!, cachedPc);

this.fetcherMock.Setup(m => m.FetchAsync(cachedPc, It.IsAny<CancellationToken>())).ReturnsAsync(
failure ? FetchResult.Failure(fetchedPc, "network error") : FetchResult.NotModified(fetchedPc));
failure ? FetchResult.Failure(fetchedPc, RefreshErrorCode.HttpRequestFailure, "network error") : FetchResult.NotModified(fetchedPc));

var config = PollingModes.AutoPoll(pollInterval, maxInitWaitTime);
var service = new AutoPollConfigService(config,
Expand All @@ -770,9 +823,13 @@ public async Task AutoPollConfigService_GetConfig_ReturnsExpiredConfigWhenCantRe

// Allow some time for other initalization callbacks to execute.
using var cts = new CancellationTokenSource();
var task = await Task.WhenAny(clientReadyTcs.Task, Task.Delay(maxInitWaitTime, cts.Token));
var clientReadyTask = Task.Run(async () => await clientReadyTcs.Task);
var task = await Task.WhenAny(clientReadyTask, Task.Delay(maxInitWaitTime, cts.Token));
cts.Cancel();
clientReadyCalled = task == clientReadyTcs.Task && task.Status == TaskStatus.RanToCompletion;
clientReadyCalled = task == clientReadyTask && task.Status == TaskStatus.RanToCompletion;

// Wait for the hook event handlers to execute (as that might not happen if the service got disposed immediately).
SpinWait.SpinUntil(() => configFetchedEvents.TryPeek(out _), TimeSpan.FromSeconds(1));
}

// Assert
Expand All @@ -782,6 +839,12 @@ public async Task AutoPollConfigService_GetConfig_ReturnsExpiredConfigWhenCantRe
this.fetcherMock.Verify(m => m.FetchAsync(cachedPc, It.IsAny<CancellationToken>()), Times.Once);

Assert.IsTrue(clientReadyCalled);

Assert.IsTrue(configFetchedEvents.Count > 0);
Assert.IsTrue(configFetchedEvents.TryDequeue(out var configFetchedEvent));
Assert.IsFalse(configFetchedEvent.IsInitiatedByUser);
Assert.AreEqual(failure, !configFetchedEvent.Result.IsSuccess);
Assert.AreEqual(failure ? RefreshErrorCode.HttpRequestFailure : RefreshErrorCode.None, configFetchedEvent.Result.ErrorCode);
}

[DataRow(false)]
Expand All @@ -801,6 +864,9 @@ public async Task LazyLoadConfigService_GetConfig_ReturnsCachedConfigWhenCachedC
var clientReadyEventCount = 0;
hooks.ClientReady += (s, e) => Interlocked.Increment(ref clientReadyEventCount);

var configFetchedEventCount = 0;
hooks.ConfigFetched += (s, e) => Interlocked.Increment(ref configFetchedEventCount);

var cache = new InMemoryConfigCache();
cache.Set(null!, cachedPc);

Expand Down Expand Up @@ -841,6 +907,8 @@ public async Task LazyLoadConfigService_GetConfig_ReturnsCachedConfigWhenCachedC
}

Assert.AreEqual(1, Volatile.Read(ref clientReadyEventCount));

Assert.AreEqual(0, Volatile.Read(ref configFetchedEventCount));
}

[DataRow(false)]
Expand All @@ -860,6 +928,9 @@ public async Task LazyLoadConfigService_GetConfig_FetchesConfigWhenCachedConfigI
var clientReadyEventCount = 0;
hooks.ClientReady += (s, e) => Interlocked.Increment(ref clientReadyEventCount);

var configFetchedEvents = new ConcurrentQueue<ConfigFetchedEventArgs>();
hooks.ConfigFetched += (s, e) => configFetchedEvents.Enqueue(e);

var cache = new InMemoryConfigCache();
cache.Set(null!, cachedPc);

Expand Down Expand Up @@ -900,5 +971,11 @@ public async Task LazyLoadConfigService_GetConfig_FetchesConfigWhenCachedConfigI
}

Assert.AreEqual(1, Volatile.Read(ref clientReadyEventCount));

Assert.IsTrue(configFetchedEvents.Count > 0);
Assert.IsTrue(configFetchedEvents.TryDequeue(out var configFetchedEvent));
Assert.IsFalse(configFetchedEvent.IsInitiatedByUser);
Assert.IsTrue(configFetchedEvent.Result.IsSuccess);
Assert.AreEqual(RefreshErrorCode.None, configFetchedEvent.Result.ErrorCode);
}
}
2 changes: 1 addition & 1 deletion src/ConfigCat.Client.Tests/ConfigV2EvaluationTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ public void PrerequisiteFlagCircularDependencyTest(string key, string dependency
var logger = new Mock<IConfigCatLogger>().Object.AsWrapper();
var evaluator = new RolloutEvaluator(logger);

var ex = Assert.ThrowsException<InvalidOperationException>(() => evaluator.Evaluate<object?>(config!.Settings, key, defaultValue: null, user: null, remoteConfig: null, logger));
var ex = Assert.ThrowsException<InvalidConfigModelException>(() => evaluator.Evaluate<object?>(config!.Settings, key, defaultValue: null, user: null, remoteConfig: null, logger));

StringAssert.Contains(ex.Message, "Circular dependency detected");
StringAssert.Contains(ex.Message, dependencyCycle);
Expand Down
2 changes: 1 addition & 1 deletion src/ConfigCat.Client.Tests/EvaluationTestsBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public void GetValue_WithIncompatibleDefaultValueType_ShouldThrowWithImprovedErr
this.logger,
};

var ex = Assert.ThrowsException<InvalidOperationException>(() =>
var ex = Assert.ThrowsException<EvaluationErrorException>(() =>
{
try { EvaluateMethodDefinition.MakeGenericMethod(settingClrType).Invoke(null, args); }
catch (TargetInvocationException ex) { throw ex.InnerException!; }
Expand Down
4 changes: 3 additions & 1 deletion src/ConfigCat.Client.Tests/HttpConfigFetcherTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public void HttpConfigFetcher_WithCustomHttpClientHandler_HandlersDisposeShouldN
}

[TestMethod]
public async Task HttpConfigFetcher_ResponseHttpCodeIsUnexpected_ShouldReturnsPassedConfig()
public async Task HttpConfigFetcher_ResponseHttpCodeIsUnexpected_ShouldReturnPassedConfig()
{
// Arrange

Expand All @@ -68,6 +68,7 @@ public async Task HttpConfigFetcher_ResponseHttpCodeIsUnexpected_ShouldReturnsPa
// Assert

Assert.IsTrue(actual.IsFailure);
Assert.AreEqual(RefreshErrorCode.InvalidSdkKey, actual.ErrorCode);
Assert.IsNotNull(actual.ErrorMessage);
Assert.IsNull(actual.ErrorException);
Assert.AreNotSame(lastConfig, actual.Config);
Expand Down Expand Up @@ -96,6 +97,7 @@ public async Task HttpConfigFetcher_ThrowAnException_ShouldReturnPassedConfig()
// Assert

Assert.IsTrue(actual.IsFailure);
Assert.AreEqual(RefreshErrorCode.HttpRequestFailure, actual.ErrorCode);
Assert.IsNotNull(actual.ErrorMessage);
Assert.AreSame(exception, actual.ErrorException);
Assert.AreEqual(lastConfig, actual.Config);
Expand Down
62 changes: 57 additions & 5 deletions src/ConfigCat.Client.Tests/OverrideTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
using System.IO;
using System.Linq;
using System.Threading.Tasks;
using ConfigCat.Client.Evaluation;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;

#if USE_NEWTONSOFT_JSON
using JsonValue = Newtonsoft.Json.Linq.JValue;
Expand Down Expand Up @@ -266,6 +266,7 @@ public void LocalOnly()
Assert.IsTrue(client.GetValue("nonexisting", false));

Assert.IsFalse(refreshResult.IsSuccess);
Assert.AreEqual(RefreshErrorCode.LocalOnlyClient, refreshResult.ErrorCode);
StringAssert.Contains(refreshResult.ErrorMessage, nameof(OverrideBehaviour.LocalOnly));
Assert.IsNull(refreshResult.ErrorException);
}
Expand Down Expand Up @@ -333,6 +334,7 @@ public async Task LocalOnly_Async()
Assert.IsTrue(client.GetValue("nonexisting", false));

Assert.IsFalse(refreshResult.IsSuccess);
Assert.AreEqual(RefreshErrorCode.LocalOnlyClient, refreshResult.ErrorCode);
StringAssert.Contains(refreshResult.ErrorMessage, nameof(OverrideBehaviour.LocalOnly));
Assert.IsNull(refreshResult.ErrorException);
}
Expand Down Expand Up @@ -576,14 +578,37 @@ public void OverrideValueTypeMismatchShouldBeHandledCorrectly_Dictionary(object
options.FlagOverrides = FlagOverrides.LocalDictionary(dictionary, OverrideBehaviour.LocalOnly);
});

var method = typeof(IConfigCatClient).GetMethod(nameof(IConfigCatClient.GetValue))!
var method = typeof(IConfigCatClient).GetMethod(nameof(IConfigCatClient.GetValueDetails))!
.GetGenericMethodDefinition()
.MakeGenericMethod(defaultValue.GetType());

var actualEvaluatedValue = method.Invoke(client, new[] { key, defaultValue, null });
var actualEvaluatedValueDetails = (EvaluationDetails)method.Invoke(client, new[] { key, defaultValue, null })!;
var actualEvaluatedValue = actualEvaluatedValueDetails.Value;
var actualEvaluatedValues = client.GetAllValues(user: null);

Assert.AreEqual(expectedEvaluatedValue, actualEvaluatedValue);
if (!defaultValue.Equals(expectedEvaluatedValue))
{
Assert.IsFalse(actualEvaluatedValueDetails.IsDefaultValue);
Assert.AreEqual(EvaluationErrorCode.None, actualEvaluatedValueDetails.ErrorCode);
Assert.IsNull(actualEvaluatedValueDetails.ErrorMessage);
Assert.IsNull(actualEvaluatedValueDetails.ErrorException);
}
else
{
Assert.IsTrue(actualEvaluatedValueDetails.IsDefaultValue);
Assert.IsNotNull(actualEvaluatedValueDetails.ErrorMessage);
if (overrideValue.ToSettingValue(out _).HasUnsupportedValue)
{
Assert.AreEqual(EvaluationErrorCode.InvalidConfigModel, actualEvaluatedValueDetails.ErrorCode);
Assert.IsInstanceOfType(actualEvaluatedValueDetails.ErrorException, typeof(InvalidConfigModelException));
}
else
{
Assert.AreEqual(EvaluationErrorCode.SettingValueTypeMismatch, actualEvaluatedValueDetails.ErrorCode);
Assert.IsInstanceOfType(actualEvaluatedValueDetails.ErrorException, typeof(EvaluationErrorException));
}
}

overrideValue.ToSettingValue(out var overrideValueSettingType);
var expectedEvaluatedValues = new KeyValuePair<string, object?>[]
Expand Down Expand Up @@ -638,14 +663,41 @@ public void OverrideValueTypeMismatchShouldBeHandledCorrectly_SimplifiedConfig(s
options.FlagOverrides = FlagOverrides.LocalFile(filePath, autoReload: false, OverrideBehaviour.LocalOnly);
});

var method = typeof(IConfigCatClient).GetMethod(nameof(IConfigCatClient.GetValue))!
var method = typeof(IConfigCatClient).GetMethod(nameof(IConfigCatClient.GetValueDetails))!
.GetGenericMethodDefinition()
.MakeGenericMethod(defaultValue.GetType());

var actualEvaluatedValue = method.Invoke(client, new[] { key, defaultValue, null });
var actualEvaluatedValueDetails = (EvaluationDetails)method.Invoke(client, new[] { key, defaultValue, null })!;
var actualEvaluatedValue = actualEvaluatedValueDetails.Value;
var actualEvaluatedValues = client.GetAllValues(user: null);

Assert.AreEqual(expectedEvaluatedValue, actualEvaluatedValue);
if (!defaultValue.Equals(expectedEvaluatedValue))
{
Assert.IsFalse(actualEvaluatedValueDetails.IsDefaultValue);
Assert.AreEqual(EvaluationErrorCode.None, actualEvaluatedValueDetails.ErrorCode);
Assert.IsNull(actualEvaluatedValueDetails.ErrorMessage);
Assert.IsNull(actualEvaluatedValueDetails.ErrorException);
}
else
{
Assert.IsTrue(actualEvaluatedValueDetails.IsDefaultValue);
Assert.IsNotNull(actualEvaluatedValueDetails.ErrorMessage);
#if USE_NEWTONSOFT_JSON
if (overrideValue is not JsonValue overrideJsonValue || overrideJsonValue.ToSettingValue(out _).HasUnsupportedValue)
#else
if (overrideValue.ToSettingValue(out _).HasUnsupportedValue)
#endif
{
Assert.AreEqual(EvaluationErrorCode.InvalidConfigModel, actualEvaluatedValueDetails.ErrorCode);
Assert.IsInstanceOfType(actualEvaluatedValueDetails.ErrorException, typeof(InvalidConfigModelException));
}
else
{
Assert.AreEqual(EvaluationErrorCode.SettingValueTypeMismatch, actualEvaluatedValueDetails.ErrorCode);
Assert.IsInstanceOfType(actualEvaluatedValueDetails.ErrorException, typeof(EvaluationErrorException));
}
}

var unwrappedOverrideValue = overrideValue is JsonValue jsonValue
? jsonValue.ToSettingValue(out var overrideValueSettingType)
Expand Down
Loading

0 comments on commit 241634c

Please sign in to comment.