Refactor frontend service for unit tests (#521)

* Refactor frontend service for unit tests

* Add more tests

* Fix

* Fix

internal/app/frontend/frontend_service.go

@@ -51,6 +51,10 @@ func (s *frontendService) CreateTicket(ctx context.Context, req *pb.CreateTicket
 		return nil, status.Errorf(codes.InvalidArgument, "ticket cannot be nil")
 	}
 
+	return doCreateTicket(ctx, req, s.store)
+}
+
+func doCreateTicket(ctx context.Context, req *pb.CreateTicketRequest, store statestore.Service) (*pb.CreateTicketResponse, error) {
 	// Generate a ticket id and create a Ticket in state storage
 	ticket, ok := proto.Clone(req.Ticket).(*pb.Ticket)
 	if !ok {
@@ -59,7 +63,7 @@ func (s *frontendService) CreateTicket(ctx context.Context, req *pb.CreateTicket
 	}
 
 	ticket.Id = xid.New().String()
-	err := s.store.CreateTicket(ctx, ticket)
+	err := store.CreateTicket(ctx, ticket)
 	if err != nil {
 		logger.WithFields(logrus.Fields{
 			"error":  err.Error(),
@@ -68,7 +72,7 @@ func (s *frontendService) CreateTicket(ctx context.Context, req *pb.CreateTicket
 		return nil, err
 	}
 
-	err = s.store.IndexTicket(ctx, ticket)
+	err = store.IndexTicket(ctx, ticket)
 	if err != nil {
 		logger.WithFields(logrus.Fields{
 			"error":  err.Error(),
@@ -83,44 +87,52 @@ func (s *frontendService) CreateTicket(ctx context.Context, req *pb.CreateTicket
 // DeleteTicket removes the Ticket from the configured indexes, thereby removing
 // it from matchmaking pool. It also lazily removes the ticket from state storage.
 func (s *frontendService) DeleteTicket(ctx context.Context, req *pb.DeleteTicketRequest) (*pb.DeleteTicketResponse, error) {
-	// Deindex this Ticket to remove it from matchmaking pool.
-	err := s.store.DeindexTicket(ctx, req.TicketId)
+	err := doDeleteTicket(ctx, req.GetTicketId(), s.store)
 	if err != nil {
-		logger.WithFields(logrus.Fields{
-			"error": err.Error(),
-			"id":    req.TicketId,
-		}).Error("failed to deindex the ticket")
 		return nil, err
 	}
-
-	// Kick off delete but don't wait for it to complete.
-	go s.deleteTicket(req.TicketId)
 	return &pb.DeleteTicketResponse{}, nil
 }
 
-// deleteTicket is a 'lazy' ticket delete that should be called after a ticket
-// has been deindexed.
-func (s *frontendService) deleteTicket(id string) {
-	err := s.store.DeleteTicket(context.Background(), id)
+func doDeleteTicket(ctx context.Context, id string, store statestore.Service) error {
+	// Deindex this Ticket to remove it from matchmaking pool.
+	err := store.DeindexTicket(ctx, id)
 	if err != nil {
 		logger.WithFields(logrus.Fields{
 			"error": err.Error(),
 			"id":    id,
-		}).Error("failed to delete the ticket")
-		return
+		}).Error("failed to deindex the ticket")
+		return err
 	}
 
-	// TODO: If other redis queues are implemented or we have custom index fields
-	// created by Open Match, those need to be cleaned up here.
+	//'lazy' ticket delete that should be called after a ticket
+	// has been deindexed.
+	go func() {
+		err := store.DeleteTicket(context.Background(), id)
+		if err != nil {
+			logger.WithFields(logrus.Fields{
+				"error": err.Error(),
+				"id":    id,
+			}).Error("failed to delete the ticket")
+			return
+		}
+		// TODO: If other redis queues are implemented or we have custom index fields
+		// created by Open Match, those need to be cleaned up here.
+	}()
+	return nil
 }
 
 // GetTicket returns the Ticket associated with the specified Ticket id.
 func (s *frontendService) GetTicket(ctx context.Context, req *pb.GetTicketRequest) (*pb.Ticket, error) {
-	ticket, err := s.store.GetTicket(ctx, req.TicketId)
+	return doGetTickets(ctx, req.GetTicketId(), s.store)
+}
+
+func doGetTickets(ctx context.Context, id string, store statestore.Service) (*pb.Ticket, error) {
+	ticket, err := store.GetTicket(ctx, id)
 	if err != nil {
 		logger.WithFields(logrus.Fields{
 			"error": err.Error(),
-			"id":    req.TicketId,
+			"id":    id,
 		}).Error("failed to get the ticket")
 		return nil, err
 	}
@@ -135,35 +147,42 @@ func (s *frontendService) GetAssignments(req *pb.GetAssignmentsRequest, stream p
 	for {
 		select {
 		case <-ctx.Done():
-			return nil
+			return ctx.Err()
 		default:
-			var currAssignment *pb.Assignment
-			callback := func(assignment *pb.Assignment) error {
-				if currAssignment == nil ||
-					currAssignment.Connection != assignment.Connection ||
-					currAssignment.Properties != assignment.Properties ||
-					currAssignment.Error != assignment.Error {
-					currAssignment, ok := proto.Clone(assignment).(*pb.Assignment)
-					if !ok {
-						logger.Error("failed to cast assignment object")
-						return status.Error(codes.Internal, "failed to cast the assignment object")
-					}
-
-					err := stream.Send(&pb.GetAssignmentsResponse{Assignment: currAssignment})
-					if err != nil {
-						logger.WithError(err).Error("failed to send Redis response to grpc server")
-						return status.Errorf(codes.Aborted, err.Error())
-					}
-				}
-				return nil
+			sender := func(assignment *pb.Assignment) error {
+				return stream.Send(&pb.GetAssignmentsResponse{Assignment: assignment})
 			}
-
-			err := s.store.GetAssignments(ctx, req.TicketId, callback)
-			if err != nil {
-				return err
-			}
-
-			return nil
+			return doGetAssignments(ctx, req.GetTicketId(), sender, s.store)
 		}
 	}
 }
+
+func doGetAssignments(ctx context.Context, id string, sender func(*pb.Assignment) error, store statestore.Service) error {
+	var currAssignment *pb.Assignment
+	callback := func(assignment *pb.Assignment) error {
+		if currAssignment == nil ||
+			currAssignment.Connection != assignment.Connection ||
+			currAssignment.Properties != assignment.Properties ||
+			currAssignment.Error != assignment.Error {
+			currAssignment, ok := proto.Clone(assignment).(*pb.Assignment)
+			if !ok {
+				logger.Error("failed to cast assignment object")
+				return status.Error(codes.Internal, "failed to cast the assignment object")
+			}
+
+			err := sender(currAssignment)
+			if err != nil {
+				logger.WithError(err).Error("failed to send Redis response to grpc server")
+				return status.Errorf(codes.Aborted, err.Error())
+			}
+		}
+		return nil
+	}
+
+	err := store.GetAssignments(ctx, id, callback)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}

internal/app/frontend/frontend_service_test.go

@@ -16,6 +16,7 @@ package frontend
 
 import (
 	"context"
+	"regexp"
 	"testing"
 	"time"
 
@@ -30,6 +31,54 @@ import (
 	statestoreTesting "open-match.dev/open-match/internal/statestore/testing"
 )
 
+func TestDoCreateTickets(t *testing.T) {
+	store, closer := statestoreTesting.NewStoreServiceForTesting(t, viper.New())
+	defer closer()
+
+	normalCtx := context.Background()
+	cancelledCtx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	testTicket := &pb.Ticket{
+		Properties: &structpb.Struct{
+			Fields: map[string]*structpb.Value{
+				"test-property": {Kind: &structpb.Value_NumberValue{NumberValue: 1}},
+			},
+		},
+	}
+
+	tests := []struct {
+		description string
+		ctx         context.Context
+		shouldErr   bool
+	}{
+		{
+			description: "expect error with canceled context",
+			ctx:         cancelledCtx,
+			shouldErr:   true,
+		},
+		{
+			description: "expect normal return with default context",
+			ctx:         normalCtx,
+			shouldErr:   false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			res, err := doCreateTicket(test.ctx, &pb.CreateTicketRequest{Ticket: testTicket}, store)
+
+			assert.Equal(t, test.shouldErr, err != nil)
+			if err == nil {
+				matched, err := regexp.MatchString(`[0-9a-v]{20}`, res.GetTicket().GetId())
+				assert.True(t, matched)
+				assert.Nil(t, err)
+				assert.Equal(t, testTicket.GetProperties().GetFields()["test-property"].GetNumberValue(), res.GetTicket().GetProperties().GetFields()["test-property"].GetNumberValue())
+			}
+		})
+	}
+}
+
 // validateTicket validates that the fetched ticket is identical to the expected ticket.
 func validateTicket(t *testing.T, got *pb.Ticket, want *pb.Ticket) {
 	assert.Equal(t, got.Id, want.Id)