Skip to content

Commit 94b6a84

Browse files
martincostellomartincostello
authored
[gRPC] Fix flaky tests (#3301)
1 parent f9433a9 commit 94b6a84

File tree

4 files changed

+53
-69
lines changed

4 files changed

+53
-69
lines changed

‎test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreClientInterceptorTests.cs‎

Lines changed: 28 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public class GrpcCoreClientInterceptorTests
2020
/// <summary>
2121
/// A bogus server uri.
2222
/// </summary>
23-
private const string BogusServerUri = "dns:i.dont.exist:77923";
23+
private const string BogusServerUri = "dns:i.do.not.exist:77923";
2424

2525
/// <summary>
2626
/// The default metadata func.
@@ -32,34 +32,28 @@ public class GrpcCoreClientInterceptorTests
3232
/// </summary>
3333
/// <returns>A task.</returns>
3434
[Fact]
35-
public async Task AsyncUnarySuccess()
36-
{
35+
public async Task AsyncUnarySuccess() =>
3736
await TestHandlerSuccess(FoobarService.MakeUnaryAsyncRequest, DefaultMetadataFunc());
38-
}
3937

4038
/// <summary>
4139
/// Validates a failed AsyncUnary call because the endpoint isn't there.
4240
/// </summary>
4341
/// <returns>A task.</returns>
4442
[Fact]
45-
public async Task AsyncUnaryUnavailable()
46-
{
43+
public async Task AsyncUnaryUnavailable() =>
4744
await TestHandlerFailure(
4845
FoobarService.MakeUnaryAsyncRequest,
4946
StatusCode.Unavailable,
5047
validateErrorDescription: false,
5148
BogusServerUri);
52-
}
5349

5450
/// <summary>
5551
/// Validates a failed AsyncUnary call because the service returned an error.
5652
/// </summary>
5753
/// <returns>A task.</returns>
5854
[Fact]
59-
public async Task AsyncUnaryFail()
60-
{
55+
public async Task AsyncUnaryFail() =>
6156
await TestHandlerFailure(FoobarService.MakeUnaryAsyncRequest);
62-
}
6357

6458
/// <summary>
6559
/// Validates a failed AsyncUnary call because the client is disposed before completing the RPC.
@@ -80,34 +74,28 @@ static void MakeRequest(Foobar.FoobarClient client)
8074
/// </summary>
8175
/// <returns>A task.</returns>
8276
[Fact]
83-
public async Task ClientStreamingSuccess()
84-
{
77+
public async Task ClientStreamingSuccess() =>
8578
await TestHandlerSuccess(FoobarService.MakeClientStreamingRequest, DefaultMetadataFunc());
86-
}
8779

8880
/// <summary>
8981
/// Validates a failed ClientStreaming call when the service is unavailable.
9082
/// </summary>
9183
/// <returns>A task.</returns>
9284
[Fact]
93-
public async Task ClientStreamingUnavailable()
94-
{
85+
public async Task ClientStreamingUnavailable() =>
9586
await TestHandlerFailure(
9687
FoobarService.MakeClientStreamingRequest,
9788
StatusCode.Unavailable,
9889
validateErrorDescription: false,
9990
BogusServerUri);
100-
}
10191

10292
/// <summary>
10393
/// Validates a failed ClientStreaming call.
10494
/// </summary>
10595
/// <returns>A task.</returns>
10696
[Fact]
107-
public async Task ClientStreamingFail()
108-
{
97+
public async Task ClientStreamingFail() =>
10998
await TestHandlerFailure(FoobarService.MakeClientStreamingRequest);
110-
}
11199

112100
/// <summary>
113101
/// Validates a failed ClientStreaming call because the client is disposed before completing the RPC.
@@ -128,20 +116,16 @@ static void MakeRequest(Foobar.FoobarClient client)
128116
/// </summary>
129117
/// <returns>A task.</returns>
130118
[Fact]
131-
public async Task ServerStreamingSuccess()
132-
{
119+
public async Task ServerStreamingSuccess() =>
133120
await TestHandlerSuccess(FoobarService.MakeServerStreamingRequest, DefaultMetadataFunc());
134-
}
135121

136122
/// <summary>
137123
/// Validates a failed ServerStreaming call.
138124
/// </summary>
139125
/// <returns>A task.</returns>
140126
[Fact]
141-
public async Task ServerStreamingFail()
142-
{
127+
public async Task ServerStreamingFail() =>
143128
await TestHandlerFailure(FoobarService.MakeServerStreamingRequest);
144-
}
145129

146130
/// <summary>
147131
/// Validates a failed ServerStreaming call because the client is disposed before completing the RPC.
@@ -162,34 +146,28 @@ static void MakeRequest(Foobar.FoobarClient client)
162146
/// </summary>
163147
/// <returns>A task.</returns>
164148
[Fact]
165-
public async Task DuplexStreamingSuccess()
166-
{
149+
public async Task DuplexStreamingSuccess() =>
167150
await TestHandlerSuccess(FoobarService.MakeDuplexStreamingRequest, DefaultMetadataFunc());
168-
}
169151

170152
/// <summary>
171153
/// Validates a failed DuplexStreaming call when the service is unavailable.
172154
/// </summary>
173155
/// <returns>A task.</returns>
174156
[Fact]
175-
public async Task DuplexStreamingUnavailable()
176-
{
157+
public async Task DuplexStreamingUnavailable() =>
177158
await TestHandlerFailure(
178159
FoobarService.MakeDuplexStreamingRequest,
179160
StatusCode.Unavailable,
180161
validateErrorDescription: false,
181162
BogusServerUri);
182-
}
183163

184164
/// <summary>
185165
/// Validates a failed DuplexStreaming call.
186166
/// </summary>
187167
/// <returns>A task.</returns>
188168
[Fact]
189-
public async Task DuplexStreamingFail()
190-
{
169+
public async Task DuplexStreamingFail() =>
191170
await TestHandlerFailure(FoobarService.MakeDuplexStreamingRequest);
192-
}
193171

194172
/// <summary>
195173
/// Validates a failed DuplexStreaming call because the client is disposed before completing the RPC.
@@ -223,17 +201,19 @@ public async Task DownstreamInterceptorActivityAccess()
223201
parentActivity.Start();
224202

225203
// Order of interceptor invocation will be ClientTracingInterceptor -> MetadataInjector
226-
callInvoker = callInvoker.Intercept(
227-
metadata =>
228-
{
229-
// This Func is called as part of an internal MetadataInjector interceptor created by gRPC Core.
230-
Assert.True(Activity.Current?.Source == GrpcCoreInstrumentation.ActivitySource);
231-
Assert.Equal(parentActivity.Id, Activity.Current.ParentId);
204+
callInvoker = callInvoker.Intercept(metadata =>
205+
{
206+
// This Func is called as part of an internal MetadataInjector interceptor created by gRPC Core.
207+
var activity = Activity.Current;
208+
209+
Assert.NotNull(activity);
210+
Assert.Equal(activity.Source, GrpcCoreInstrumentation.ActivitySource);
211+
Assert.Equal(parentActivity.Id, activity.ParentId);
232212

233-
// Set a tag on the Activity and make sure we can see it afterwards
234-
Activity.Current.SetTag("foo", "bar");
235-
return metadata;
236-
});
213+
// Set a tag on the Activity and make sure we can see it afterwards
214+
activity.SetTag("foo", "bar");
215+
return metadata;
216+
});
237217

238218
var testTags = new TestActivityTags();
239219
var interceptorOptions = new ClientTracingInterceptorOptions { AdditionalTags = testTags.Tags };
@@ -316,8 +296,7 @@ internal static void ValidateCommonActivityTags(
316296
Assert.NotNull(activity);
317297
Assert.NotNull(activity.Tags);
318298

319-
// The activity was stopped
320-
Assert.True(activity.Duration != default);
299+
Assert.True(activity.IsStopped);
321300

322301
// TagObjects contain non string values
323302
// Tags contains only string values
@@ -336,8 +315,8 @@ internal static void ValidateCommonActivityTags(
336315
{
337316
// all methods accept a request and return a single response
338317
Assert.NotNull(activity.Events);
339-
var requestMessage = activity.Events.FirstOrDefault(ae => ae.Name == FoobarService.DefaultRequestMessage.GetType().Name);
340-
var responseMessage = activity.Events.FirstOrDefault(ae => ae.Name == FoobarService.DefaultResponseMessage.GetType().Name);
318+
var requestMessage = activity.Events.FirstOrDefault(ae => ae.Name == nameof(FoobarRequest));
319+
var responseMessage = activity.Events.FirstOrDefault(ae => ae.Name == nameof(FoobarResponse));
341320

342321
static void ValidateCommonEventAttributes(ActivityEvent activityEvent)
343322
{
@@ -501,7 +480,7 @@ private static async Task TestHandlerFailure(
501480
]);
502481

503482
using var activityListener = new InterceptorActivityListener(testTags);
504-
await Assert.ThrowsAsync<RpcException>(async () => await clientRequestFunc(client, null).ConfigureAwait(false));
483+
await Assert.ThrowsAsync<RpcException>(() => clientRequestFunc(client, null));
505484

506485
var activity = activityListener.Activity;
507486
ValidateCommonActivityTags(activity, statusCode, interceptorOptions.RecordMessageEvents, interceptorOptions.RecordException);

‎test/OpenTelemetry.Instrumentation.GrpcCore.Tests/TestActivityTags.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ public TestActivityTags()
2424
/// Checks whether the activity has test tags.
2525
/// </summary>
2626
/// <param name="activity">The activity.</param>
27-
/// <returns>Returns true if the activty has test tags, false otherwise.</returns>
27+
/// <returns>Returns true if the activity has test tags, false otherwise.</returns>
2828
internal bool HasTestTags(Activity activity)
2929
{
3030
Guard.ThrowIfNull(activity);

‎test/OpenTelemetry.Instrumentation.GrpcNetClient.Tests/GrpcTests.client.cs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -333,9 +333,7 @@ public void GrpcDoesNotPropagateContextWithSuppressInstrumentationOptionSetToFal
333333
var clientActivity = exportedItems.Single(activity => activity.OperationName == OperationNameGrpcOut);
334334

335335
Assert.Equal(clientActivity.ParentSpanId, parentActivity.SpanId);
336-
337-
// Propagator is not called
338-
Assert.False(isPropagatorCalled);
336+
Assert.False(isPropagatorCalled, "Propagator was called.");
339337
}
340338
finally
341339
{
@@ -385,7 +383,7 @@ public void GrpcClientInstrumentationRespectsSdkSuppressInstrumentation()
385383
// If suppressed, activity is not emitted and
386384
// propagation is also not performed.
387385
Assert.Single(exportedItems);
388-
Assert.False(isPropagatorCalled);
386+
Assert.False(isPropagatorCalled, "Propagator was called.");
389387
}
390388
finally
391389
{

‎test/OpenTelemetry.Instrumentation.GrpcNetClient.Tests/GrpcTests.server.cs‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,13 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(string? enableGrp
5757
using var channel = GrpcChannel.ForAddress(uri);
5858
var client = new Greeter.GreeterClient(channel);
5959
var returnMsg = client.SayHello(new HelloRequest()).Message;
60-
Assert.False(string.IsNullOrEmpty(returnMsg));
60+
61+
Assert.NotNull(returnMsg);
62+
Assert.NotEmpty(returnMsg);
6163

6264
WaitForExporterToReceiveItems(exportedItems, 1);
63-
Assert.Single(exportedItems);
64-
var activity = exportedItems[0];
65+
66+
var activity = Assert.Single(exportedItems);
6567

6668
Assert.Equal(ActivityKind.Server, activity.Kind);
6769

@@ -95,7 +97,9 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(string? enableGrp
9597
}
9698

9799
#if NET
98-
[Theory(Skip = "Skipping for .NET 6 and higher due to bug #3023")]
100+
[Theory(Skip = "https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1778")]
101+
#else
102+
[Theory]
99103
#endif
100104
[InlineData(null)]
101105
[InlineData("true")]
@@ -110,11 +114,11 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewAc
110114
Sdk.SetDefaultTextMapPropagator(new Extensions.Propagators.B3Propagator());
111115
var exportedItems = new List<Activity>();
112116
var configuration = new ConfigurationBuilder()
113-
.AddInMemoryCollection(new Dictionary<string, string?>
114-
{
115-
["OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION"] = enableGrpcAspNetCoreSupport,
116-
})
117-
.Build();
117+
.AddInMemoryCollection(new Dictionary<string, string?>
118+
{
119+
["OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION"] = enableGrpcAspNetCoreSupport,
120+
})
121+
.Build();
118122

119123
using var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder()
120124
.ConfigureServices(services => services.AddSingleton<IConfiguration>(configuration))
@@ -137,8 +141,8 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewAc
137141
client.SayHello(new HelloRequest(), headers);
138142

139143
WaitForExporterToReceiveItems(exportedItems, 1);
140-
Assert.Single(exportedItems);
141-
var activity = exportedItems[0];
144+
145+
var activity = Assert.Single(exportedItems);
142146

143147
Assert.Equal(ActivityKind.Server, activity.Kind);
144148

@@ -189,14 +193,17 @@ private static void WaitForExporterToReceiveItems(List<Activity> itemsReceived,
189193
{
190194
// We need to let End callback execute as it is executed AFTER response was returned.
191195
// In unit tests environment there may be a lot of parallel unit tests executed, so
192-
// giving some breezing room for the End callback to complete
193-
Assert.True(SpinWait.SpinUntil(
196+
// giving some breathing room for the End callback to complete.
197+
var timeout = TimeSpan.FromSeconds(5);
198+
var satisfied = SpinWait.SpinUntil(
194199
() =>
195200
{
196-
Thread.Sleep(10);
201+
Thread.Sleep(25);
197202
return itemsReceived.Count >= itemCount;
198203
},
199-
TimeSpan.FromSeconds(1)));
204+
timeout);
205+
206+
Assert.True(satisfied, $"{itemCount} item(s) not received within {timeout}.");
200207
}
201208
}
202209
#endif

0 commit comments

Comments
 (0)