googleinterns / cloud-operations-api-mock

Apache License 2.0
5 stars 3 forks source link

Refactor out server logic so it can be used as a library (raw use) #12

Closed the-ericwang35 closed 4 years ago

the-ericwang35 commented 4 years ago

This change allows users to spin up the server/clients through a couple lines of code using the cloudmock package rather than having to manually spin up the server, similar to the raw use of Moto. Sample use in a unit test:

func TestXxx(t *testing.T) {
    mock := cloudmock.NewCloudMock()

    // Run tests here using mock.MockTraceClient and mock.MockMetricClient

    mock.Shutdown()
}
the-ericwang35 commented 4 years ago
  1. Try to keep symmetry with the method names. Either StartMockServer/StopMockServer or StartupMockServer/ShutdownMockServer.
  2. Is it possible to change this so StartMockClients does the work of creating the mock server too? This would make it a bit simpler to use in tests.
  3. It would be good if clients don't have to specify and address for unit testing. If you leave the port blank or zero then an unused port is taken: net.Listen("tcp", "127.0.0.1:0"). That way we can remove the address parameter. This also solves some issues with concurrency. (What happens if we running test in parallel?)
  4. Is there a reason why the mock clients are bundled? Does it make sense for them to be separated?
func StartMockTraceClient() (*grpc.ClientConn, cloudtrace.TraceServiceClient)
func StartMockMetricClient() (*grpc.ClientConn, monitoring.MetricServiceClient)
  1. Alternatively to (4), would it make sense to group the returned values into a struct?
type MockClient struct {
  conn *grpc.ClientConn
  TraceServiceClient cloudtrace.TraceServiceClient
  MetricServiceClient monitoring.MetricServiceClient
}

func NewMockClient() *MockClient
func (client *MockClient) Shutdown()
  1. Yep, I changed them to NewCloudMock and Shutdown as per your suggestion in (5)
  2. Yep, I combined them into a single NewCloudMock function
  3. Done, changed it to net.Listen("tcp", "127.0.0.1:0"). I tried running some tests concurrently and it appears to be working fine (gRPC automatically handles requests in a new goroutine), but when we start storing stuff in memory we'll need some mutexes
  4. I went with the approach of grouping the return values into a struct that you mentioned in (5). This will make it easier for the user to start up everything in one line, as opposed to separately starting up the server/clients