fix: tests reaching the web #1161
Conversation
|
It breaks on Ubuntu |
| ); | ||
| var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath)); | ||
| _options.DiagnosticLogger?.LogDebug("Moving unprocessed file back to cache: {0} to {1}.", | ||
| filePath, destinationPath); |
There was a problem hiding this comment.
nit-pick but I believe the below one will be slightly more clear to the end-user.
| filePath, destinationPath); | |
| _options.DiagnosticLogger?.LogDebug("Moving unprocessed file back from cache {0} to {1}.", | |
| filePath, destinationPath); | |
There was a problem hiding this comment.
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
| #if !NET461 && !NETSTANDARD2_0 | ||
| await | ||
| #endif | ||
| using var stream = new MemoryStream(); | ||
| using var stream = new MemoryStream(); |
There was a problem hiding this comment.
| #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 |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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"); | |||
There was a problem hiding this comment.
flaky since https://example.com exists but not https://fake.tld.
There was a problem hiding this comment.
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.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
No description provided.