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.

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

fix: tests reaching the web #1161

Merged
merged 8 commits into from
Aug 25, 2021
Merged

fix: tests reaching the web #1161

merged 8 commits into from
Aug 25, 2021

Conversation

bruno-garcia
Copy link
Member

No description provided.

@lucas-zimerman
Copy link
Collaborator

lucas-zimerman commented Aug 23, 2021

It breaks on Ubuntu

Error: Sentry.AspNetCore.Tests.SentryHttpMessageHandlerBuilderFilterTests.Generated_client_sends_Sentry_trace_header_automatically: System.Net.Http.HttpRequestException : Name or service not known (fake.tld:443)
  Failed Sentry.AspNetCore.Tests.SentryHttpMessageHandlerBuilderFilterTests.Generated_client_sends_Sentry_trace_header_automatically [431 ms]

CHANGELOG.md Outdated Show resolved Hide resolved
);
var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath));
_options.DiagnosticLogger?.LogDebug("Moving unprocessed file back to cache: {0} to {1}.",
filePath, destinationPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit-pick but I believe the below one will be slightly more clear to the end-user.

Suggested change
filePath, destinationPath);
_options.DiagnosticLogger?.LogDebug("Moving unprocessed file back from cache {0} to {1}.",
filePath, destinationPath);

Copy link
Member Author

Choose a reason for hiding this comment

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

You suggestion adds another call to the logger?
Code here is moving for "processing" back to the "cache" folder. So I think the original message makes more sense

Comment on lines +15 to +18
#if !NET461 && !NETSTANDARD2_0
await
#endif
using var stream = new MemoryStream();
using var stream = new MemoryStream();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#if !NET461 && !NETSTANDARD2_0
await
#endif
using var stream = new MemoryStream();
using var stream = new MemoryStream();
#if !NET461 && !NETSTANDARD2_0
await using var stream = new MemoryStream();
#else
using var stream = new MemoryStream();
#endif

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is it beneficial to add a #else block with the same statement over only adding await?

If we agreed on this new style lets do a single pass and replace it everything. The approach I used is already the standard and there are many usages

@@ -4,18 +4,18 @@
using System.Threading.Tasks;
using Sentry.Protocol.Envelopes;

namespace Sentry.Tests.Helpers
namespace Sentry.Internal
{
internal static class SerializableExtensions
Copy link
Collaborator

Choose a reason for hiding this comment

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

I may be wrong, but moving a static code from the Test project to the Main project isn't going to include an unused static extension to the Main SDK instead of limiting it to the Test projects.

@@ -74,11 +74,10 @@ public async Task Generated_client_sends_Sentry_trace_header_automatically()
.GetRequiredService<IHttpClientFactory>()
.CreateClient();

await httpClient.GetAsync("https://example.com");
await httpClient.GetAsync("https://fake.tld");
Copy link
Collaborator

Choose a reason for hiding this comment

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

flaky since https://example.com exists but not https://fake.tld.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow. Why is it flaky?

The goal is to make sure we're not relying on a real DNS name and Internet to resolve it.

I ran these tests offline and the build failed due to such tests.

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

Great changes!.
Left some comments but overall looks ok (except the broken test that needs to be fixed before merging)

@bruno-garcia bruno-garcia enabled auto-merge (squash) August 25, 2021 00:15
@codecov-commenter
Copy link

Codecov Report

Merging #1161 (5474209) into main (bccf22b) will decrease coverage by 1.35%.
The diff coverage is 70.99%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1161      +/-   ##
==========================================
- Coverage   80.55%   79.20%   -1.36%     
==========================================
  Files         204      205       +1     
  Lines        6727     6641      -86     
  Branches     1495     1508      +13     
==========================================
- Hits         5419     5260     -159     
- Misses        821      898      +77     
+ Partials      487      483       -4     
Impacted Files Coverage Δ
src/Sentry/SentryOptions.cs 90.12% <0.00%> (+0.55%) ⬆️
src/Sentry/Span.cs 73.61% <0.00%> (+1.00%) ⬆️
src/Sentry/SpanTracer.cs 88.37% <0.00%> (+2.00%) ⬆️
src/Sentry/Transaction.cs 81.87% <0.00%> (+0.54%) ⬆️
src/Sentry/Internal/SdkComposer.cs 43.90% <25.00%> (+1.68%) ⬆️
src/Sentry/Internal/Hub.cs 65.67% <28.57%> (+1.24%) ⬆️
src/Sentry/Internal/Http/CachingTransport.cs 79.28% <47.36%> (+1.14%) ⬆️
src/Sentry.AspNetCore/SentryTracingMiddleware.cs 73.68% <50.00%> (+0.60%) ⬆️
src/Sentry/GlobalSessionManager.cs 69.43% <66.66%> (+0.23%) ⬆️
.../Internal/DuplicateEventDetectionEventProcessor.cs 86.20% <66.66%> (-5.46%) ⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bccf22b...5474209. Read the comment docs.

@bruno-garcia bruno-garcia merged commit 58cb042 into main Aug 25, 2021
@bruno-garcia bruno-garcia deleted the ref/tests-diagnostics branch August 25, 2021 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants