feat: add FeatureEnabled to Variant and fix spec testing

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