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

Refactoring tests to support both external and loopback servers #74579

Closed

Conversation

greenEkatherine
Copy link
Contributor

@greenEkatherine greenEkatherine commented Aug 25, 2022

Fixes #73849

I did an example on one test SendReceive_Concurrent_Success_Base how I can run an existing scenarios on both external and loopback servers with different versions, security settings and attributes (like running on browser or outerloop).

Please share your thoughts about the implementation because I would like to make changes before I apply it for the remaining scenarios.

@ghost
Copy link

ghost commented Aug 25, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #73849

I did an example on one test how I can run an existing scenarios on both external and loopback servers with different versions, security settings and attributes (like running on browser or outerloop).

Please share your thoughts about the implementation because I would like to make changes before I apply it for the remaining scenarios.

Author: greenEkatherine
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@ghost
Copy link

ghost commented Aug 25, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #73849

I did an example on one test how I can run an existing scenarios on both external and loopback servers with different versions, security settings and attributes (like running on browser or outerloop).

Please share your thoughts about the implementation because I would like to make changes before I apply it for the remaining scenarios.

Author: greenEkatherine
Assignees: greenEkatherine
Labels:

area-System.Net

Milestone: -

@karelz karelz added this to the 8.0.0 milestone Aug 25, 2022
@greenEkatherine
Copy link
Contributor Author

@MihaZupan @CarnaViire could you please have a look on the proposed changes? I would like to apply feedback earlier because after I add changes to other tests it will require a lot of copy paste.

@MihaZupan
Copy link
Member

I think the pattern we use in Http tests is cleaner.
See HttpClientHandlerTestBase.cs, HttpClientHandlerTestBase.SocketsHttpHandler.cs, GenericLoopbackServer.cs.

We could add something like the following to ClientWebSocketTestBase:

protected virtual Version UseVersion => HttpVersion.Version11;
protected virtual bool UseRemoteServer => false;

protected Task CreateEchoServerAsync(Func<Uri, Task> clientFunc)
{
    using var server = CreateServer(UseVersion, UseRemoteServer, ...);
    await clientFunc(server.Uri);
}

protected Task<ClientWebSocket> GetConnectedWebSocket(Uri serverUri) =>
    GetConnectedWebSocket(serverUri, TimeOutMilliseconds, _output, UseVersion));

This can then be used in tests like

public async Task MyTest()
{
    await CreateEchoServerAsync(async uri =>
    {
        using ClientWebSocket cws = await GetConnectedWebSocket(uri);
        await cws.DoStuffAsync();
    });
}

This has a few benefits:

  • Tests are easier to read - you don't deal with client and server tasks manually
  • Tests don't have to deal with version / type of server directly
  • Avoids adding [Theory] combinations to every test for remote/version
    • Different combinations are defined at the test class level like we have for custom invoker vs HttpClient right now
  • Makes managing the lifetime of the server trivial since you never directly hand it out
    • Currently you are returning the server uri and task, but the server itself isn't getting cleaned up

@ghost ghost closed this Oct 13, 2022
@ghost
Copy link

ghost commented Oct 13, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2022
@dotnet dotnet unlocked this conversation Nov 22, 2022
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Nice, this looks much more familiar to the Http tests

@greenEkatherine greenEkatherine marked this pull request as ready for review January 5, 2023 16:29
@CarnaViire
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

There seem to be WebSocket test failures, could you please check them?

Link="CommonTest\System\Security\Cryptography\PlatformSupport.cs" />
<Compile Include="$(CommonTestPath)System\Threading\Tasks\TaskTimeoutExtensions.cs"
Link="Common\System\Threading\Tasks\TaskTimeoutExtensions.cs" />
<Compile Include="$(CommonTestPath)System\Net\Capability.Security.cs" Link="Common\System\Net\Capability.Security.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

Are the changes in this file needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were files added and others are auto-formatted

@@ -419,6 +636,37 @@ public async Task SendReceive_Concurrent_Success(Uri server)
}
}

[ConditionalFact(nameof(WebSocketsSupported))]
public async Task SendReceive_Concurrent_Success_Base()
Copy link
Member

Choose a reason for hiding this comment

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

I assume you don't plan to update all the WebSocket tests to new format in this PR, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 23, 2023
Co-authored-by: Natalia Kondratyeva <[email protected]>
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 25, 2023
@greenEkatherine
Copy link
Contributor Author

There seem to be WebSocket test failures, could you please check them?

For now there failed one unrelated build and local http2 web socket tests on Libraries Test Run release coreclr windows x86 Release. I see the intermittent failures indicating that web socket is in closed state.

@CarnaViire
Copy link
Member

I will take over this PR, approximately in a month. Marking as draft for now.

@CarnaViire CarnaViire marked this pull request as draft January 31, 2023 20:39
@ghost ghost closed this Mar 2, 2023
@ghost
Copy link

ghost commented Mar 2, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests with LoopbackServer for web sockets
4 participants