From 8731406f64ec374ef6347d1a0a068ca3c11635cd Mon Sep 17 00:00:00 2001 From: James Hartig Date: Fri, 8 Dec 2023 11:42:24 -0600 Subject: [PATCH 1/2] feat: add FeatureEnabled to Variant and fix spec testing FeatureEnabled is similar to Enabled except in the case where the feature is enabled but there are no variants defined. This follows the client specification case [1]. Fixes #159 [1] https://github.com/Unleash/client-specification/blob/c0169d7ace35db66cdf41a7b1b4e390a4a843c3b/specifications/08-variants.json#L448 --- api/feature.go | 1 + api/variant.go | 15 ++++++--- client.go | 12 +++++-- client_test.go | 89 +++++++++++++++++++++++++++++++++++++++++++++++++ spec_test.go | 19 ++++++++--- unleash_test.go | 11 +++--- 6 files changed, 132 insertions(+), 15 deletions(-) diff --git a/api/feature.go b/api/feature.go index 8bd8c04..1ab1e1e 100644 --- a/api/feature.go +++ b/api/feature.go @@ -89,6 +89,7 @@ func (vc VariantCollection) GetVariant(ctx *context.Context) *Variant { variant = &v.Variant } variant.Enabled = true + variant.FeatureEnabled = true return variant } return DISABLED_VARIANT diff --git a/api/variant.go b/api/variant.go index bf8b229..8211dd2 100644 --- a/api/variant.go +++ b/api/variant.go @@ -3,8 +3,9 @@ package api import "github.com/Unleash/unleash-client-go/v4/context" var DISABLED_VARIANT = &Variant{ - Name: "disabled", - Enabled: false, + Name: "disabled", + Enabled: false, + FeatureEnabled: false, } type Payload struct { @@ -26,8 +27,11 @@ type Variant struct { Name string `json:"name"` // Payload is the value of the variant payload Payload Payload `json:"payload"` - // Enabled indicates whether the feature which is extend by this variant was enabled or not. + // Enabled indicates whether the variant is enabled. This is only false when + // it's a default variant. Enabled bool `json:"enabled"` + // FeatureEnabled indicates whether the Feature for this variant is enabled. + FeatureEnabled bool `json:"featureEnabled"` } type VariantInternal struct { @@ -81,7 +85,10 @@ func (o Override) matchValue(ctx *context.Context) bool { return false } -// Get default variant if no variant is found +// Get default variant if feature is not found or if the feature is disabled. +// +// Rather than checking against this particular variant you should be checking +// the returned variant's Enabled and FeatureEnabled properties. func GetDefaultVariant() *Variant { return DISABLED_VARIANT } diff --git a/client.go b/client.go index a2a6c57..b757de6 100644 --- a/client.go +++ b/client.go @@ -31,6 +31,14 @@ var defaultStrategies = []strategy.Strategy{ *s.NewFlexibleRolloutStrategy(), } +// disabledVariantFeatureEnabled is similar to api.DISABLED_VARIANT but we want +// to discourage public usage so it's internal until there's a need to expose it. +var disabledVariantFeatureEnabled = &api.Variant{ + Name: "disabled", + Enabled: false, + FeatureEnabled: true, +} + // Client is a structure representing an API client of an Unleash server. type Client struct { errorChannels @@ -381,7 +389,7 @@ func (uc *Client) isParentDependencySatisfied(feature *api.Feature, context cont func (uc *Client) GetVariant(feature string, options ...VariantOption) *api.Variant { variant := uc.getVariantWithoutMetrics(feature, options...) defer func() { - uc.metrics.countVariants(feature, variant.Enabled, variant.Name) + uc.metrics.countVariants(feature, variant.FeatureEnabled, variant.Name) }() return variant } @@ -436,7 +444,7 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt } if len(f.Variants) == 0 { - return defaultVariant + return disabledVariantFeatureEnabled } return api.VariantCollection{ diff --git a/client_test.go b/client_test.go index ba5331c..8950ce6 100644 --- a/client_test.go +++ b/client_test.go @@ -740,6 +740,8 @@ func TestClient_VariantShouldRespectConstraint(t *testing.T) { assert.True(variant.Enabled) + assert.True(variant.FeatureEnabled) + variantFromResolver := client.GetVariant(feature, WithVariantContext(context.Context{ Properties: map[string]string{"custom-id": "custom-ctx", "semver": "3.2.2", "age": "18", "domain": "unleashtest"}, }), WithVariantResolver(func(featureName string) *api.Feature { @@ -753,6 +755,8 @@ func TestClient_VariantShouldRespectConstraint(t *testing.T) { assert.True(variantFromResolver.Enabled) + assert.True(variantFromResolver.FeatureEnabled) + assert.True(gock.IsDone(), "there should be no more mocks") } @@ -855,6 +859,8 @@ func TestClient_VariantShouldFailWhenSegmentConstraintsDontMatch(t *testing.T) { assert.False(variant.Enabled) + assert.False(variant.FeatureEnabled) + variantFromResolver := client.GetVariant(feature, WithVariantContext(context.Context{ Properties: map[string]string{"custom-id": "custom-ctx", "semver": "3.2.2", "age": "18", "domain": "unleashtest"}, }), WithVariantResolver(func(featureName string) *api.Feature { @@ -868,6 +874,8 @@ func TestClient_VariantShouldFailWhenSegmentConstraintsDontMatch(t *testing.T) { assert.False(variantFromResolver.Enabled) + assert.False(variantFromResolver.FeatureEnabled) + assert.True(gock.IsDone(), "there should be no more mocks") } @@ -953,6 +961,8 @@ func TestClient_ShouldFavorStrategyVariantOverFeatureVariant(t *testing.T) { assert.True(strategyVariant.Enabled) + assert.True(strategyVariant.FeatureEnabled) + assert.Equal("strategyVariantName", strategyVariant.Name) assert.True(gock.IsDone(), "there should be no more mocks") @@ -1051,7 +1061,86 @@ func TestClient_ShouldReturnOldVariantForNonMatchingStrategyVariant(t *testing.T assert.True(strategyVariant.Enabled) + assert.True(strategyVariant.FeatureEnabled) + assert.Equal("willBeSelected", strategyVariant.Name) assert.True(gock.IsDone(), "there should be no more mocks") } + +func TestClient_VariantFromEnabledFeatureWithNoVariants(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() + + variant := client.GetVariant(feature, WithVariantContext(context.Context{})) + + assert.False(variant.Enabled) + + assert.True(variant.FeatureEnabled) + + assert.Equal(disabledVariantFeatureEnabled, variant) + + variantFromResolver := client.GetVariant(feature, WithVariantContext(context.Context{}), 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.True(variantFromResolver.FeatureEnabled) + + assert.Equal(disabledVariantFeatureEnabled, variantFromResolver) + + assert.True(gock.IsDone(), "there should be no more mocks") +} diff --git a/spec_test.go b/spec_test.go index 3355c77..ac4b2ef 100644 --- a/spec_test.go +++ b/spec_test.go @@ -36,11 +36,18 @@ type TestCase struct { ExpectedResult bool `json:"expectedResult"` } +type expectedVariantResult struct { + api.Variant + // SpecFeatureEnabled represents the spec's feature_enabled field which has a + // different JSON field name than api.Variant + SpecFeatureEnabled bool `json:"feature_enabled"` +} + type VariantTestCase struct { - Description string `json:"description"` - Context context.Context `json:"context"` - ToggleName string `json:"toggleName"` - ExpectedResult *api.Variant `json:"expectedResult"` + Description string `json:"description"` + Context context.Context `json:"context"` + ToggleName string `json:"toggleName"` + ExpectedResult *expectedVariantResult `json:"expectedResult"` } type Runner interface { @@ -88,7 +95,9 @@ func (vtc VariantTestCase) RunWithClient(client *Client) func(*testing.T) { result := client.GetVariant(vtc.ToggleName, WithVariantContext(vtc.Context)) wg.Wait() assert.Equal(t, vtc.ExpectedResult.Enabled, result.Enabled) - assert.Equal(t, vtc.ExpectedResult, client.GetVariant(vtc.ToggleName)) + // copy over the FeatureEnabled field with different JSON tag + vtc.ExpectedResult.FeatureEnabled = vtc.ExpectedResult.SpecFeatureEnabled + assert.Equal(t, &vtc.ExpectedResult.Variant, result) } } diff --git a/unleash_test.go b/unleash_test.go index 1bf07a0..1e96730 100644 --- a/unleash_test.go +++ b/unleash_test.go @@ -48,14 +48,17 @@ func Test_withVariants(t *testing.T) { t.Fail() } - feature := unleash.GetVariant("Demo") - if feature.Enabled == false { + variant := unleash.GetVariant("Demo") + if variant.Enabled == false { + t.Fatalf("Expected variant to be enabled") + } + if variant.FeatureEnabled == false { t.Fatalf("Expected feature to be enabled") } - if feature.Name != "small" && feature.Name != "medium" { + if variant.Name != "small" && variant.Name != "medium" { t.Fatalf("Expected one of the variant names") } - if feature.Payload.Value != "35" && feature.Payload.Value != "55" { + if variant.Payload.Value != "35" && variant.Payload.Value != "55" { t.Fatalf("Expected one of the variant payloads") } err = unleash.Close() From 62d39b7335c9915eccb350bfb5bdf625afb80d89 Mon Sep 17 00:00:00 2001 From: Fredrik Oseberg Date: Wed, 13 Dec 2023 17:51:38 +0100 Subject: [PATCH 2/2] fix: add workflow to build PRs --- .github/workflows/build-prs.yml | 46 +++++++++++++++++++++++++++++++++ .github/workflows/build.yml | 5 +++- 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/build-prs.yml diff --git a/.github/workflows/build-prs.yml b/.github/workflows/build-prs.yml new file mode 100644 index 0000000..64bb9e1 --- /dev/null +++ b/.github/workflows/build-prs.yml @@ -0,0 +1,46 @@ +name: Build PRs + +on: + pull_request: + +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + version: [1.13, 1.15, 1.16] + steps: + - uses: actions/checkout@v2 + name: Checkout code + - uses: actions/checkout@v2 + name: Checkout client specifications + with: + repository: Unleash/client-specification + ref: refs/tags/v5.1.0 + path: testdata/client-specification + - uses: actions/setup-go@v2 + name: Setup go + with: + go-version: ${{ matrix.version }} + - name: Get deps + run: go get -t -v ./... + - name: Go vet + run: go vet ./... + - name: Run spec tests + run: go test -v ./... -tags='norace' + - name: Run all tests with race detection + timeout-minutes: 1 + run: go test -race -covermode atomic -coverprofile=profile.cov -v ./... -tags='!norace' + - name: Send coverage + uses: shogo82148/actions-goveralls@v1 + with: + path-to-profile: profile.cov + flag-name: Go-${{ matrix.version }} + parallel: true + finish: + needs: build + runs-on: ubuntu-latest + steps: + - uses: shogo82148/actions-goveralls@v1 + with: + parallel-finished: true \ No newline at end of file diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d9f3be2..e68f2af 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,6 +1,9 @@ name: Build -on: [push] +on: + push: + branches: + - main jobs: build: