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

[dev] Update dependencies from dotnet/arcade #4896

Merged
merged 7 commits into from
Feb 12, 2024

Conversation

dotnet-maestro[bot]
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Jan 22, 2024

This pull request updates the following dependencies

From https://github.com/dotnet/arcade

  • Subscription: 6bb35465-7476-40b8-10e6-08dbd53e3775
  • Build: 20240202.4
  • Date Produced: February 2, 2024 10:58:06 PM UTC
  • Commit: 2fb543a45580400a559b5ae41c96a815ea14dac5
  • Branch: refs/heads/main
Microsoft Reviewers: Open in CodeFlow

dotnet-maestro bot and others added 2 commits January 22, 2024 14:19
…119.2

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 9.0.0-beta.24062.5 -> To Version 9.0.0-beta.24069.2
@sebastienros sebastienros enabled auto-merge (squash) January 23, 2024 01:28
@RussKie
Copy link
Member

RussKie commented Jan 25, 2024

Tests needs fixing

##[error]test/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware.Tests/Latency/RequestLatencyTelemetryMiddlewareTests.cs(45,9): error xUnit2028: (NETCORE_ENGINEERING_TELEMETRY=Build) Using Assert.NotEmpty with an instance of Microsoft.Extensions.Primitives.StringValues is problematic, because it is implicitly cast to a string, not a collection. Check the length with .Count instead. (https://xunit.net/xunit.analyzers/rules/xUnit2028)

@xakep139
Copy link
Contributor

Tests needs fixing

##[error]test/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware.Tests/Latency/RequestLatencyTelemetryMiddlewareTests.cs(45,9): error xUnit2028: (NETCORE_ENGINEERING_TELEMETRY=Build) Using Assert.NotEmpty with an instance of Microsoft.Extensions.Primitives.StringValues is problematic, because it is implicitly cast to a string, not a collection. Check the length with .Count instead. (https://xunit.net/xunit.analyzers/rules/xUnit2028)

@ananthsr can you please take a look?

…126.5

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 9.0.0-beta.24062.5 -> To Version 9.0.0-beta.24076.5
@amcasey
Copy link
Member

amcasey commented Feb 1, 2024

aspnetcore is also having issues with this update, but they're specific to the way that repo is built and probably don't apply here. Is it just the tests that need to be straightened out?

dotnet-maestro bot and others added 2 commits February 5, 2024 14:13
…202.4

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk
 From Version 9.0.0-beta.24062.5 -> To Version 9.0.0-beta.24102.4
…t.Extensions.Primitives.StringValues is problematic, because it is implicitly cast to a string, not a collection
@RussKie
Copy link
Member

RussKie commented Feb 7, 2024

@sebastienros @geeknoid could you please help with the failing test Delay_InvalidArgs:

public void Delay_InvalidArgs()
{
var timeProvider = new FakeTimeProvider();
_ = Assert.ThrowsAsync<ArgumentOutOfRangeException>(() => timeProvider.Delay(TimeSpan.FromTicks(-1), CancellationToken.None));
_ = Assert.ThrowsAsync<ArgumentOutOfRangeException>(() => timeProvider.Delay(_infiniteTimeout, CancellationToken.None));
}

https://dev.azure.com/dnceng-public/public/_build/results?buildId=557733&view=ms.vss-test-web.build-test-results-tab&runId=13292592&resultId=100029&paneView=debug

@sebastienros
Copy link
Member

sebastienros commented Feb 7, 2024

@geeknoid

In the runtime we have:

https://github.com/dotnet/runtime/blob/b4491522e03c64a70fcac66dbe5a836e1a8dfffe/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs#L5602-L5606

https://github.com/dotnet/runtime/blob/b4491522e03c64a70fcac66dbe5a836e1a8dfffe/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs#L5656-L5662

var timeout = TimeSpan.FromTicks(-1);
long totalMilliseconds = (long)timeout.TotalMilliseconds; // -> 0

So this will not throw, and I don't see how the change here would be the reason. I'd like to know why it worked. The test should use something that is bigger than a millisecond (negative), and I wonder why the runtime uses strict comparison. Even if they checked for ticks, it would need to be -2 ticks.

@RussKie RussKie requested a review from geeknoid February 9, 2024 06:15
@RussKie
Copy link
Member

RussKie commented Feb 9, 2024

Assigning to @geeknoid for now.

@geeknoid
Copy link
Member

geeknoid commented Feb 9, 2024

Looking at the code in Task.cs, I don't know how the test ever worked. I looked at the change history in Task.cs and nothing seemingly affected this code path. Another place I thought maybe the exception was coming from is https://github.com/dotnet/extensions/blob/b7f81954a2c94cf0b8f00e7bbb6e4c5a6bd57cb7/src/Libraries/Microsoft.Extensions.TimeProvider.Testing/Timer.cs#L36C1-L38C1 but that's also just checking for -1.

@geeknoid
Copy link
Member

geeknoid commented Feb 9, 2024

Found it.

The test code was lacking await after the assertions, so the behavior was a bit undefined and the exceptions were never really asserted. This test was written originally as part of the FakeClock version of the code in R9, and it doesn't really add value in this current context (since it's actually verifying the implementation of code in dotnet/runtime), I just went ahead and deleted the case completely.

#4934

@dotnet-comment-bot
Copy link
Collaborator

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.AspNetCore.Diagnostics.Middleware 99 100

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=562123&view=codecoverage-tab

@dotnet-maestro dotnet-maestro bot merged commit 047854e into dev Feb 12, 2024
7 checks passed
@dotnet-maestro dotnet-maestro bot deleted the darc-dev-543bd71e-67f5-4d05-81a6-9febf41877cb branch February 12, 2024 00:10
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants