From 2d278e34400d6a57cda68ed9c7b5028ab1cfaf76 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 18 Dec 2023 09:18:48 +0100 Subject: [PATCH 1/7] fix: start adding test cases / impls --- client.go | 5 +++ client_test.go | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/client.go b/client.go index b757de6..12d9ab5 100644 --- a/client.go +++ b/client.go @@ -416,6 +416,11 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt } if !strategyResult.Enabled { + if opts.variantFallbackFunc != nil { + return opts.variantFallbackFunc(feature, ctx) + } else if opts.variantFallback != nil { + return opts.variantFallback + } return defaultVariant } diff --git a/client_test.go b/client_test.go index 8950ce6..7c7ee6d 100644 --- a/client_test.go +++ b/client_test.go @@ -1144,3 +1144,91 @@ func TestClient_VariantFromEnabledFeatureWithNoVariants(t *testing.T) { assert.True(gock.IsDone(), "there should be no more mocks") } + +// test cases (each with fallback and fallbackfunc): +// 1. feature is disabled -> FeatureEnabled : false +// 2. feature doesn't exist -> FeatureEnabled : false +// 3. feature is enabled but no variants -> FeatureEnabled : true +// 4. test strategy variants too + + +func TestGetVariantWithFallbackVariant(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + MatchHeader("UNLEASH-APPNAME", mockAppName). + MatchHeader("UNLEASH-INSTANCEID", mockInstanceId). + Reply(200) + + feature := "feature-disabled" + features := []api.Feature{ + { + Name: feature, + Description: "feature-desc", + Enabled: false, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "default-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "default", + }, + }, + }, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + mockListener := &MockedListener{} + mockListener.On("OnReady").Return() + mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")) + mockListener.On("OnCount", feature, false).Return() + mockListener.On("OnError").Return() + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(mockListener), + ) + + assert.NoError(err) + client.WaitForReady() + + fallbackVariant := api.Variant{ + Name: "fallback-variant", + } + + variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) + + assert.False(variant.Enabled) + + assert.False(variant.FeatureEnabled) + + assert.Equal(fallbackVariant, *variant) + + variantFromResolver := client.GetVariant(feature, WithVariantFallback(&fallbackVariant), WithVariantResolver(func(featureName string) *api.Feature { + if featureName == features[0].Name { + return &features[0] + } else { + t.Fatalf("the feature name passed %s was not the expected one %s", featureName, features[0].Name) + return nil + } + })) + + assert.False(variantFromResolver.Enabled) + + assert.False(variantFromResolver.FeatureEnabled) + + assert.Equal(fallbackVariant, *variantFromResolver) + + assert.True(gock.IsDone(), "there should be no more mocks") +} From 00292ade1af4dc1280437475144c39ca8501237b Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 18 Dec 2023 10:48:35 +0100 Subject: [PATCH 2/7] fix: test that it works for functions too --- client_test.go | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/client_test.go b/client_test.go index 7c7ee6d..fc931c4 100644 --- a/client_test.go +++ b/client_test.go @@ -1151,7 +1151,6 @@ func TestClient_VariantFromEnabledFeatureWithNoVariants(t *testing.T) { // 3. feature is enabled but no variants -> FeatureEnabled : true // 4. test strategy variants too - func TestGetVariantWithFallbackVariant(t *testing.T) { assert := assert.New(t) defer gock.OffAll() @@ -1215,20 +1214,13 @@ func TestGetVariantWithFallbackVariant(t *testing.T) { assert.Equal(fallbackVariant, *variant) - variantFromResolver := client.GetVariant(feature, WithVariantFallback(&fallbackVariant), WithVariantResolver(func(featureName string) *api.Feature { - if featureName == features[0].Name { - return &features[0] - } else { - t.Fatalf("the feature name passed %s was not the expected one %s", featureName, features[0].Name) - return nil - } - })) - - assert.False(variantFromResolver.Enabled) + fallbackFunc := func(feature string, ctx *context.Context) *api.Variant { + return &fallbackVariant + } - assert.False(variantFromResolver.FeatureEnabled) + variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc)) - assert.Equal(fallbackVariant, *variantFromResolver) + assert.Equal(fallbackVariant, *variantWithFallbackFunc) assert.True(gock.IsDone(), "there should be no more mocks") } From 72f7c0be6271db62efa79bde0399c91614b3546f Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 18 Dec 2023 11:07:36 +0100 Subject: [PATCH 3/7] fix: update test name --- client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client_test.go b/client_test.go index fc931c4..87e98bc 100644 --- a/client_test.go +++ b/client_test.go @@ -1151,7 +1151,7 @@ func TestClient_VariantFromEnabledFeatureWithNoVariants(t *testing.T) { // 3. feature is enabled but no variants -> FeatureEnabled : true // 4. test strategy variants too -func TestGetVariantWithFallbackVariant(t *testing.T) { +func TestGetVariantWithFallbackVariantWhenFeatureDisabled(t *testing.T) { assert := assert.New(t) defer gock.OffAll() From fd124116bdf775d9f876e9d05e52e0edbb568c67 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 18 Dec 2023 11:07:51 +0100 Subject: [PATCH 4/7] fix: handle case where flag has no variants --- client.go | 10 +++++++ client_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/client.go b/client.go index 12d9ab5..e34f895 100644 --- a/client.go +++ b/client.go @@ -449,6 +449,16 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt } if len(f.Variants) == 0 { + if opts.variantFallbackFunc != nil { + variant := opts.variantFallbackFunc(feature, ctx) + if variant != nil { + variant.FeatureEnabled = true + } + return variant + } else if opts.variantFallback != nil { + opts.variantFallback.FeatureEnabled = true + return opts.variantFallback + } return disabledVariantFeatureEnabled } diff --git a/client_test.go b/client_test.go index 87e98bc..92ecd7e 100644 --- a/client_test.go +++ b/client_test.go @@ -1224,3 +1224,77 @@ func TestGetVariantWithFallbackVariantWhenFeatureDisabled(t *testing.T) { assert.True(gock.IsDone(), "there should be no more mocks") } + +func TestGetVariantWithFallbackVariantWhenFeatureEnabledButNoVariants(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + MatchHeader("UNLEASH-APPNAME", mockAppName). + MatchHeader("UNLEASH-INSTANCEID", mockInstanceId). + Reply(200) + + feature := "feature-no-variants" + features := []api.Feature{ + { + Name: feature, + Description: "feature-desc", + Enabled: true, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "default-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "default", + }, + }, + }, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + mockListener := &MockedListener{} + mockListener.On("OnReady").Return() + mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")) + mockListener.On("OnCount", feature, true).Return() + mockListener.On("OnError").Return() + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(mockListener), + ) + + assert.NoError(err) + client.WaitForReady() + + fallbackVariant := api.Variant{ + Name: "fallback-variant", + } + + variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) + + assert.False(variant.Enabled) + + assert.True(variant.FeatureEnabled) + + assert.Equal(fallbackVariant, *variant) + + fallbackFunc := func(feature string, ctx *context.Context) *api.Variant { + return &fallbackVariant + } + + variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc)) + + assert.Equal(fallbackVariant, *variantWithFallbackFunc) + + assert.True(gock.IsDone(), "there should be no more mocks") +} From 24b850907eb50973bd120455d40847bc6883f126 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 18 Dec 2023 11:13:50 +0100 Subject: [PATCH 5/7] fix: test case when flag doesn't exist --- client_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 6 deletions(-) diff --git a/client_test.go b/client_test.go index 92ecd7e..de4c386 100644 --- a/client_test.go +++ b/client_test.go @@ -1145,12 +1145,6 @@ func TestClient_VariantFromEnabledFeatureWithNoVariants(t *testing.T) { assert.True(gock.IsDone(), "there should be no more mocks") } -// test cases (each with fallback and fallbackfunc): -// 1. feature is disabled -> FeatureEnabled : false -// 2. feature doesn't exist -> FeatureEnabled : false -// 3. feature is enabled but no variants -> FeatureEnabled : true -// 4. test strategy variants too - func TestGetVariantWithFallbackVariantWhenFeatureDisabled(t *testing.T) { assert := assert.New(t) defer gock.OffAll() @@ -1298,3 +1292,68 @@ func TestGetVariantWithFallbackVariantWhenFeatureEnabledButNoVariants(t *testing assert.True(gock.IsDone(), "there should be no more mocks") } + +// test cases (each with fallback and fallbackfunc): +// 4. test strategy variants too + +func TestGetVariantWithFallbackVariantWhenFeatureDoesntExist(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + MatchHeader("UNLEASH-APPNAME", mockAppName). + MatchHeader("UNLEASH-INSTANCEID", mockInstanceId). + Reply(200) + + feature := "feature-no-variants" + features := []api.Feature{ + {}, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + mockListener := &MockedListener{} + mockListener.On("OnReady").Return() + mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")) + mockListener.On("OnCount", feature, false).Return() + mockListener.On("OnError").Return() + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(mockListener), + ) + + assert.NoError(err) + client.WaitForReady() + + fallbackVariant := api.Variant{ + Name: "fallback-variant", + } + + variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) + + assert.False(variant.Enabled) + + assert.False(variant.FeatureEnabled) + + assert.Equal(fallbackVariant, *variant) + + fallbackFunc := func(feature string, ctx *context.Context) *api.Variant { + return &fallbackVariant + } + + variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc)) + + assert.Equal(fallbackVariant, *variantWithFallbackFunc) + + assert.True(gock.IsDone(), "there should be no more mocks") +} From 9e5b6376aacfbbf8e22181863ca4c38489f8202e Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 18 Dec 2023 11:16:01 +0100 Subject: [PATCH 6/7] fix: overwrite fallback with correct FeatureEnabled values --- client.go | 19 +++++++++++++------ client_test.go | 6 +++--- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/client.go b/client.go index e34f895..46d8012 100644 --- a/client.go +++ b/client.go @@ -417,8 +417,13 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt if !strategyResult.Enabled { if opts.variantFallbackFunc != nil { - return opts.variantFallbackFunc(feature, ctx) + variant := opts.variantFallbackFunc(feature, ctx) + if variant != nil { + variant.FeatureEnabled = false + } + return variant } else if opts.variantFallback != nil { + opts.variantFallback.FeatureEnabled = false return opts.variantFallback } return defaultVariant @@ -431,18 +436,20 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt f = uc.repository.getToggle(feature) } - if f == nil { + if f == nil || !f.Enabled { if opts.variantFallbackFunc != nil { - return opts.variantFallbackFunc(feature, ctx) + variant := opts.variantFallbackFunc(feature, ctx) + if variant != nil { + variant.FeatureEnabled = false + } + return variant } else if opts.variantFallback != nil { + opts.variantFallback.FeatureEnabled = false return opts.variantFallback } return defaultVariant } - if !f.Enabled { - return defaultVariant - } if strategyResult.Variant != nil { return strategyResult.Variant diff --git a/client_test.go b/client_test.go index de4c386..3c913ac 100644 --- a/client_test.go +++ b/client_test.go @@ -1198,6 +1198,7 @@ func TestGetVariantWithFallbackVariantWhenFeatureDisabled(t *testing.T) { fallbackVariant := api.Variant{ Name: "fallback-variant", + FeatureEnabled: true, } variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) @@ -1272,6 +1273,7 @@ func TestGetVariantWithFallbackVariantWhenFeatureEnabledButNoVariants(t *testing fallbackVariant := api.Variant{ Name: "fallback-variant", + FeatureEnabled: false, } variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) @@ -1293,9 +1295,6 @@ func TestGetVariantWithFallbackVariantWhenFeatureEnabledButNoVariants(t *testing assert.True(gock.IsDone(), "there should be no more mocks") } -// test cases (each with fallback and fallbackfunc): -// 4. test strategy variants too - func TestGetVariantWithFallbackVariantWhenFeatureDoesntExist(t *testing.T) { assert := assert.New(t) defer gock.OffAll() @@ -1337,6 +1336,7 @@ func TestGetVariantWithFallbackVariantWhenFeatureDoesntExist(t *testing.T) { fallbackVariant := api.Variant{ Name: "fallback-variant", + FeatureEnabled: true, } variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) From 0ef66e9606c9c2c55a31d9fb4bbff28d6822de08 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 18 Dec 2023 11:23:58 +0100 Subject: [PATCH 7/7] fix: unify impl --- client.go | 40 ++++++++++++++-------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/client.go b/client.go index 46d8012..1c0331e 100644 --- a/client.go +++ b/client.go @@ -415,18 +415,27 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt strategyResult = uc.isEnabled(feature, WithContext(*ctx)) } - if !strategyResult.Enabled { + getFallbackWithFeatureEnabled := func(featureEnabled bool) *api.Variant { if opts.variantFallbackFunc != nil { variant := opts.variantFallbackFunc(feature, ctx) if variant != nil { - variant.FeatureEnabled = false + variant.FeatureEnabled = featureEnabled } return variant } else if opts.variantFallback != nil { - opts.variantFallback.FeatureEnabled = false + opts.variantFallback.FeatureEnabled = featureEnabled return opts.variantFallback } + + if featureEnabled { + return disabledVariantFeatureEnabled + } return defaultVariant + + } + + if !strategyResult.Enabled { + return getFallbackWithFeatureEnabled(false) } var f *api.Feature @@ -437,36 +446,15 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt } if f == nil || !f.Enabled { - if opts.variantFallbackFunc != nil { - variant := opts.variantFallbackFunc(feature, ctx) - if variant != nil { - variant.FeatureEnabled = false - } - return variant - } else if opts.variantFallback != nil { - opts.variantFallback.FeatureEnabled = false - return opts.variantFallback - } - return defaultVariant + return getFallbackWithFeatureEnabled(false) } - if strategyResult.Variant != nil { return strategyResult.Variant } if len(f.Variants) == 0 { - if opts.variantFallbackFunc != nil { - variant := opts.variantFallbackFunc(feature, ctx) - if variant != nil { - variant.FeatureEnabled = true - } - return variant - } else if opts.variantFallback != nil { - opts.variantFallback.FeatureEnabled = true - return opts.variantFallback - } - return disabledVariantFeatureEnabled + return getFallbackWithFeatureEnabled(true) } return api.VariantCollection{