Our LatestClientAndLatestServiceBuilder tangles the concept of client and service setup into one big class, making it hard to determine the boundaries.
Results
Before
It would allows the developer to setup a test case without a service, or without a client. This resulted in the ClientAndService object having nullable Client and Service properties. Contractually, this makes it uncertain if these properties have values.
It also meant it was harder to use just the client or service by itself.
After
No More NoService
The NoService property is now replaced with a builder for specifically getting a client only.
Despite NoService being on the IClientAndServiceBuilder, all test cases except 1 (details in code comment) only ever used it on the latest client and latest service.
So we only implemented a client only version for the latest client.
Creating Clients When No Service Exists
When we create a "client only" client, we often want to actually attempt an RPC. Therefore we have to CreateAsyncClient. But this expects an endpoint (as we are trying to reach a service). But if we have not created a service, then what is the URI we should be attempting to reach?
Therefore, we added the CreateClientWithoutService method, which creates a real but unattached endpoint.
Port Forwarding in IClientAndService
We removed the PortForwarder property from the IClientAndService, as there was already a partial implementation to get this via the builder.
Like with Client and Service, means it no longer is 'nullable'.
In hindsight, this enhancement should be a separate PR. But at the time it was thought to be needed to finish this feature. It wasn't until near the end where it was clear that it could have been separate sorry 😞. Due to the time constraint of Sharpening, we kept it as one PR.
Port Forwarding - Client or Service?
The port that needs forwarding very much depends on whether we are listening or polling. If we are listening, then we need to forward the port on the service side. If we are polling, then we need to forward the port on the client side.
As a result, if we are using a port forwarder for a client and service, the builder that builds it will change depending on the type.
TCPListenerWhichKillsNewConnections
The original LatestClientAndLatestServiceBuilder used to create a TCPListenerWhichKillsNewConnections when there was no service.
We are assuming that this was done to ensure we chose a port that was free, and would not accept connections.
So to keep this behaviour, it was reworked into the LatestClient when using CreateClientWithoutService, as attempting to connect to a service that does not exist is the way this would get exercised.
How to review this PR
The meat of the change is within LatestClientBuilder, LatestServiceBuilder, and the old LatestClientAndLatestServiceBuilder. So I would recommend starting there.
Please ensure that the split is correct, and that I didn't overlook anything (or get it wrong) when I split them apart.
Quality :heavy_check_mark:
Pre-requisites
[ ] I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
[ ] I have considered informing or consulting the right people, according to the ownership map.
[ ] I have considered appropriate testing for my change.
Background
Our
LatestClientAndLatestServiceBuildertangles the concept of client and service setup into one big class, making it hard to determine the boundaries.Results
Before
It would allows the developer to setup a test case without a service, or without a client. This resulted in the
ClientAndServiceobject having nullableClientandServiceproperties. Contractually, this makes it uncertain if these properties have values.It also meant it was harder to use just the client or service by itself.
After
No More NoService
The
NoServiceproperty is now replaced with a builder for specifically getting a client only.Despite
NoServicebeing on theIClientAndServiceBuilder, all test cases except 1 (details in code comment) only ever used it on the latest client and latest service.So we only implemented a client only version for the latest client.
Creating Clients When No Service Exists
When we create a "client only" client, we often want to actually attempt an RPC. Therefore we have to
CreateAsyncClient. But this expects an endpoint (as we are trying to reach a service). But if we have not created a service, then what is the URI we should be attempting to reach?Therefore, we added the
CreateClientWithoutServicemethod, which creates a real but unattached endpoint.Port Forwarding in IClientAndService
We removed the
PortForwarderproperty from theIClientAndService, as there was already a partial implementation to get this via the builder.Like with
ClientandService, means it no longer is 'nullable'.In hindsight, this enhancement should be a separate PR. But at the time it was thought to be needed to finish this feature. It wasn't until near the end where it was clear that it could have been separate sorry 😞. Due to the time constraint of Sharpening, we kept it as one PR.
Port Forwarding - Client or Service?
The port that needs forwarding very much depends on whether we are listening or polling. If we are listening, then we need to forward the port on the service side. If we are polling, then we need to forward the port on the client side.
As a result, if we are using a port forwarder for a client and service, the builder that builds it will change depending on the type.
TCPListenerWhichKillsNewConnections
The original
LatestClientAndLatestServiceBuilderused to create aTCPListenerWhichKillsNewConnectionswhen there was no service.We are assuming that this was done to ensure we chose a port that was free, and would not accept connections.
So to keep this behaviour, it was reworked into the
LatestClientwhen usingCreateClientWithoutService, as attempting to connect to a service that does not exist is the way this would get exercised.How to review this PR
The meat of the change is within
LatestClientBuilder,LatestServiceBuilder, and the oldLatestClientAndLatestServiceBuilder. So I would recommend starting there.Please ensure that the split is correct, and that I didn't overlook anything (or get it wrong) when I split them apart.
Quality :heavy_check_mark:
Pre-requisites