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

Error reporting improvements #86

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Error reporting improvements #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d2f0b0c
Define error codes for evaluation errors (EvaluationErrorCode) and ex…
adams85 Jan 4, 2024
1d821b6
Adjust tests
adams85 Mar 25, 2024
0bab940
Add XML comments to new public APIs
adams85 Apr 15, 2024
b24c576
Add tests
adams85 Apr 15, 2024
87a63fb
Define error codes for config data refresh errors (RefreshErrorCode) …
adams85 Apr 16, 2024
acc00da
Make subscription to hooks thread-safe
adams85 Apr 16, 2024
73ab47d
Introduce ConfigFetched hook
adams85 Apr 16, 2024
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
99 changes: 97 additions & 2 deletions src/ConfigCat.Client.Tests/ConfigCatClientTests.cs
View file
Open in desktop

Large diffs are not rendered by default.

87 changes: 82 additions & 5 deletions src/ConfigCat.Client.Tests/ConfigServiceTests.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading