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

Jump to bottom

Clean up async timeout extension methods #31671

Merged
merged 7 commits into from
Apr 17, 2021
Merged

Clean up async timeout extension methods #31671

merged 7 commits into from
Apr 17, 2021

Conversation

ladeak
Copy link
Contributor

@ladeak ladeak commented Apr 10, 2021

Clean up async timeout extension methods

  • Consolidating all Timeout extension methods in Microsoft.AspNetCore.Testing project
  • Removing WithTimeout, OrTimeout and DefaultTimeout definitions in other projects
  • Using DefaultTimeout consistently instead of WithTimeout and OrTimeout
  • Keeping Timeout duration defitions unique to projects (IIS) which has larger than the usual 30 seconds elsewhere
  • Adding NETCore TargetFramwork to Microsoft.AspNetCore.Testing so .NET6 bits can be used
  • Using WaitAsync in NET6 and with if-else compiler switches to fallback to existing implementation
  • http2cat add dependency on Microsoft.AspNetCore.Testing, as far as I have seen this is providing helper classes to Functional and Integration test projects.

Addresses #16620

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 10, 2021
@ladeak
Copy link
Contributor Author

ladeak commented Apr 11, 2021 •
edited
Loading

These failing tests seems to fail even on main branch for me. Do you have any suggestions?
EDIT: pull and rebase solved the problem.

@ladeak ladeak mentioned this pull request Apr 13, 2021
Closed
Tratcher
Tratcher reviewed Apr 15, 2021
View reviewed changes
...gnalR/server/Specification.Tests/src/Microsoft.AspNetCore.SignalR.Specification.Tests.csproj Outdated
@@ -20,6 +20,7 @@
<Reference Include="Microsoft.AspNetCore.SignalR.Protocols.Json" />
<Reference Include="Microsoft.AspNetCore.SignalR.Protocols.MessagePack" />
<Reference Include="Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson" />
<Reference Include="Microsoft.AspNetCore.Testing" />
Copy link
Member

Choose a reason for hiding this comment

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

This one's a problem, Microsoft.AspNetCore.SignalR.Specification.Tests is actually a shipping package, not a test. It can't depend on Microsoft.AspNetCore.Testing which is not a shipping package.
https://www.nuget.org/packages/Microsoft.AspNetCore.SignalR.Specification.Tests

Copy link
Member

Choose a reason for hiding this comment

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

Right, this one can't use any test helpers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will double check this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both HubLifetimeManagerTestBase and ScaleoutHubLifetimeManagerTests uses DefaultTimeout heavily. Which makes me move TaskExtensions from Microsoft.AspNetCore.Testing to \Shared folder to reference the file directly to resolve this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets a little more tricky in TestClient.cs to make it compile everywhere, I have to use compiler conditions in the using statements.

src/Testing/src/TaskExtensions.cs Outdated
@@ -23,7 +68,16 @@ public static class TaskExtensions
{
return await task;
}

#if NET6_0
Copy link
Member

Choose a reason for hiding this comment

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

Everywhere:

Suggested change
#if NET6_0
#if NET6_0_OR_GREATER

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, will update

@JamesNK JamesNK merged commit 62e157b into dotnet:main Apr 17, 2021
@ghost ghost added this to the 6.0-preview5 milestone Apr 17, 2021
@JamesNK
Copy link
Member

JamesNK commented Apr 17, 2021

Thanks for the PR! It will be good to have consistent timeout methods across the repo.

@davidfowl
Copy link
Member

Solid work @ladeak!

@davidfowl
Copy link
Member

cc @stephentoub

@ladeak ladeak deleted the ladeak_WaitAsync branch April 17, 2021 06:53
@stephentoub
Copy link
Member

Nice. Thanks.

@ghost
Copy link

ghost commented Apr 19, 2021

Hi @stephentoub. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware label Jun 6, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@Tratcher Tratcher Tratcher left review comments

@halter73 halter73 halter73 approved these changes

@JamesNK JamesNK JamesNK approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@jkotalik jkotalik Awaiting requested review from jkotalik

Assignees

@jkotalik jkotalik

Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
6.0-preview5
Development

Successfully merging this pull request may close these issues.
9 participants
@ladeak @JamesNK @davidfowl @stephentoub @halter73 @Tratcher @Pilchie @jkotalik @amcasey