feat: add FeatureEnabled to Variant and fix spec testing (#166)

* 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

* fix: add workflow to build PRs

---------

Co-authored-by: Fredrik Oseberg <[email protected]>

.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

.github/workflows/build.yml

@@ -1,6 +1,9 @@
 name: Build
 
-on: [push]
+on:
+  push:
+    branches:
+      - main
 
 jobs:
   build:

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

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
 }

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{

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")
+}

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)
 	}
 }
 

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()