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

Socket platform tests #647

Merged
merged 7 commits into from
Mar 24, 2017
Merged

Socket platform tests #647

merged 7 commits into from
Mar 24, 2017

Conversation

codito
Copy link
Contributor

@codito codito commented Mar 22, 2017

Tests for SocketCommunicationManager functionality.

@msftclas
Copy link

@codito,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot


namespace Microsoft.TestPlatform.CommunicationUtilities.PlatformTests
{
public static class Program
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Yes, for every test project that targets netcoreapp. We don't include NET.Test.Sdk directly.

@harshjain2
Copy link
Contributor

More tests could be added as per these specification of TcpClient and TcpServer :
TcpClient

  1. ExclusiveAddressUse : Communication protocol currently support only one client. How will the system behave if more than one clients are connected.
  2. LingerState : For scenarios like support UAP apps, LingerState is of importance. How will the communication layer behave in case server and client are not on same machine(device, emulator, IoT, etc.)
  3. SendBufferSize, ReceiveBufferSize : How will the system behave if the data sent exceeds the default buffer size, 8192 bytes.

NoDelay : This field sounds interesting as it can significantly improve the efficiency of communication layer, specifically in remote scenarios.

TcpListener

  1. ExclusiveAddressUse : Same as for TcpClient.

@harshjain2 harshjain2 closed this Mar 22, 2017
@harshjain2 harshjain2 reopened this Mar 22, 2017
@msftclas
Copy link

@codito,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@codito
Copy link
Contributor Author

codito commented Mar 23, 2017

ExclusiveAddressUse : Communication protocol currently support only one client. How will the system behave if more than one clients are connected.

We don't allow multiple client use. One SCM per ProxyExecutionManager.

LingerState : For scenarios like support UAP apps, LingerState is of importance. How will the communication layer behave in case server and client are not on same machine(device, emulator, IoT, etc.)

This is a choice of the implementation. In our implementation we use the default; consumers of ISocketCommunicationManager don't have a way to modify this. I believe for UAP/Emulator (anywhere .NET is, LingerState will be default false (as per spec in msdn). There's no timeout. I don't think we require tests for this.

SendBufferSize, ReceiveBufferSize : How will the system behave if the data sent exceeds the default buffer size, 8192 bytes.

This is internal implementation detail. Consumers of SocketCommunicationManager cannot change this. We use a length-prefixed buffer (BinaryReader/Writer), this shouldn't create a problem. Are you suggesting we add happy-path tests?

NoDelay - is on our backlog for next PR (refactors SCM).

@AbhitejJohn
Copy link
Contributor

@codito: Are these tests running in the PR today?

@codito
Copy link
Contributor Author

codito commented Mar 23, 2017

Yes, they are running in the PR.

@codito codito merged commit 710d043 into microsoft:master Mar 24, 2017
@codito codito deleted the socket-pt branch March 24, 2017 01:53
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.

6 participants