#44 Covered the priority routing by tests & fixed issues in logic and…

… related tests

pkg/routers/router.go

@@ -34,7 +34,7 @@ func NewLangRouter(cfg *LangRouterConfig, tel *telemetry.Telemetry) (*LangRouter
 		return nil, err
 	}
 
-	routing, err := cfg.BuildRouting(models)
+	strategy, err := cfg.BuildRouting(models)
 	if err != nil {
 		return nil, err
 	}
@@ -44,7 +44,7 @@ func NewLangRouter(cfg *LangRouterConfig, tel *telemetry.Telemetry) (*LangRouter
 		Config:    cfg,
 		models:    models,
 		retry:     cfg.BuildRetry(),
-		routing:   routing,
+		routing:   strategy,
 		telemetry: tel,
 	}
 

pkg/routers/router_test.go

@@ -57,17 +57,22 @@ func TestLangRouter_Priority_PickFistHealthy(t *testing.T) {
 	}
 }
 
-func TestLangRouter_Priority_PickSecondHealthy(t *testing.T) {
-	budget := health.NewErrorBudget(3, health.SEC)
+func TestLangRouter_Priority_PickThirdHealthy(t *testing.T) {
+	budget := health.NewErrorBudget(1, health.SEC)
 	langModels := []providers.LanguageModel{
 		providers.NewLangModel(
 			"first",
-			providers.NewProviderMock([]providers.ResponseMock{{Err: &ErrNoModelAvailable}, {Msg: "2"}}),
+			providers.NewProviderMock([]providers.ResponseMock{{Err: &ErrNoModelAvailable}, {Msg: "3"}}),
 			*budget,
 		),
 		providers.NewLangModel(
 			"second",
-			providers.NewProviderMock([]providers.ResponseMock{{Msg: "1"}}),
+			providers.NewProviderMock([]providers.ResponseMock{{Err: &ErrNoModelAvailable}, {Msg: "4"}}),
+			*budget,
+		),
+		providers.NewLangModel(
+			"third",
+			providers.NewProviderMock([]providers.ResponseMock{{Msg: "1"}, {Msg: "2"}}),
 			*budget,
 		),
 	}
@@ -77,7 +82,7 @@ func TestLangRouter_Priority_PickSecondHealthy(t *testing.T) {
 		models = append(models, model)
 	}
 
-	expectedModels := []string{"second", "first"}
+	expectedModels := []string{"third", "third"}
 
 	router := LangRouter{
 		routerID:  "test_router",
@@ -101,7 +106,7 @@ func TestLangRouter_Priority_PickSecondHealthy(t *testing.T) {
 }
 
 func TestLangRouter_Priority_SuccessOnRetry(t *testing.T) {
-	budget := health.NewErrorBudget(3, health.SEC)
+	budget := health.NewErrorBudget(1, health.MILLI)
 	langModels := []providers.LanguageModel{
 		providers.NewLangModel(
 			"first",
@@ -175,7 +180,7 @@ func TestLangRouter_Priority_UnhealthyModelInThePool(t *testing.T) {
 }
 
 func TestLangRouter_Priority_AllModelsUnavailable(t *testing.T) {
-	budget := health.NewErrorBudget(3, health.SEC)
+	budget := health.NewErrorBudget(1, health.SEC)
 	langModels := []providers.LanguageModel{
 		providers.NewLangModel(
 			"first",

pkg/routers/routing/priority.go

@@ -40,17 +40,15 @@ type PriorityIterator struct {
 
 func (r PriorityIterator) Next() (providers.Model, error) {
 	models := r.models
-	idx := r.idx.Load()
 
-	for int(idx) < len(models) {
-		idx = r.idx.Load()
+	for idx := int(r.idx.Load()); idx < len(models); idx = int(r.idx.Add(1)) {
 		model := models[idx]
 
-		r.idx.Add(1)
-
-		if model.Healthy() {
-			return model, nil
+		if !model.Healthy() {
+			continue
 		}
+
+		return model, nil
 	}
 
 	return nil, ErrNoHealthyModels

pkg/routers/routing/priority_test.go

@@ -0,0 +1,60 @@
+package routing
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"glide/pkg/providers"
+)
+
+func TestPriorityRouting_PickModelsInOrder(t *testing.T) {
+	type Model struct {
+		modelID string
+		healthy bool
+	}
+
+	type TestCase struct {
+		models           []Model
+		expectedModelIDs []string
+	}
+
+	tests := map[string]TestCase{
+		"all healthy":         {[]Model{{"first", true}, {"second", true}, {"third", true}}, []string{"first", "first", "first"}},
+		"first unhealthy":     {[]Model{{"first", false}, {"second", true}, {"third", true}}, []string{"second", "second", "second"}},
+		"first two unhealthy": {[]Model{{"first", false}, {"second", false}, {"third", true}}, []string{"third", "third", "third"}},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			models := make([]providers.Model, 0, len(tc.models))
+
+			for _, model := range tc.models {
+				models = append(models, providers.NewLangModelMock(model.modelID, model.healthy))
+			}
+
+			routing := NewPriorityRouting(models)
+			iterator := routing.Iterator()
+
+			// loop three times over the whole pool to check if we return back to the begging of the list
+			for _, modelID := range tc.expectedModelIDs {
+				model, err := iterator.Next()
+				require.NoError(t, err)
+				require.Equal(t, modelID, model.ID())
+			}
+		})
+	}
+}
+
+func TestPriorityRouting_NoHealthyModels(t *testing.T) {
+	models := []providers.Model{
+		providers.NewLangModelMock("first", false),
+		providers.NewLangModelMock("second", false),
+		providers.NewLangModelMock("third", false),
+	}
+
+	routing := NewPriorityRouting(models)
+	iterator := routing.Iterator()
+
+	_, err := iterator.Next()
+	require.Error(t, err)
+}