Skip to content

Add tests for feature manager#44

Merged
linglingye001 merged 1 commit intorelease/v1.1.2from
linglingye/feature-manager-tests
Apr 16, 2026
Merged

Add tests for feature manager#44
linglingye001 merged 1 commit intorelease/v1.1.2from
linglingye/feature-manager-tests

Conversation

@linglingye001
Copy link
Copy Markdown
Member

@linglingye001 linglingye001 commented Apr 16, 2026

NewFeatureManager initialization:

  • Nil provider returns error
  • Custom filters are registered and used correctly
  • Nil filter in options doesn't cause a panic

GetFeatureNames:

  • Returns all feature flag names correctly
  • Returns empty slice for empty provider
  • Returns nil when provider errors

Unknown/missing filter:

  • Feature with unregistered filter returns disabled

RequirementType logic:

  • All — enabled when all filters return true
  • All — disabled when any filter returns false
  • Any — enabled when at least one filter returns true
  • Any — disabled when all filters return false

Filter error handling:

  • Filter returning an error propagates the error

StatusOverride:

  • Variant with StatusOverride: "Enabled" keeps feature enabled

GetVariant edge cases:

  • Non-existent feature returns error
  • Feature with no variants returns nil variant
  • Feature with no allocation returns nil variant
  • User allocation overrides default_when_enabled
  • Pointer *TargetingContext works the same as value

IsEnabledWithAppContext:

  • Non-TargetingContext app context still works
  • Provider error propagates correctly

Validation errors:

  • Empty feature flag ID
  • Invalid RequirementType
  • Variant missing name
  • Invalid StatusOverride value
  • Invalid percentile range (negative From)
  • User allocation with empty users list
  • Group allocation with empty groups list
  • Client filter missing name
  • Percentile allocation missing variant
  • User allocation missing variant
  • Group allocation missing variant

Provider errors:

  • IsEnabled, IsEnabledWithAppContext, and GetVariant all return errors when provider fails

FeatureManagement struct:

  • JSON deserialization into the FeatureManagement type works correctly

@RichardChen820 RichardChen820 requested a review from Copilot April 16, 2026 07:39
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a comprehensive set of unit tests around the FeatureManager API to validate initialization behavior, filter evaluation semantics, variant selection edge cases, and validation/provider error paths.

Changes:

  • Adds tests for NewFeatureManager initialization scenarios (nil provider, custom filters, nil filters in options).
  • Adds tests for GetFeatureNames, missing/unknown filters, requirement type evaluation (Any/All), and filter error propagation.
  • Adds tests covering variant assignment edge cases, provider error propagation across APIs, and JSON deserialization into FeatureManagement.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +97 to +102
expected := []string{"Feature1", "Feature2", "Feature3"}
for i, name := range names {
if name != expected[i] {
t.Errorf("Expected name %q at index %d, got %q", expected[i], i, name)
}
}
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestGetFeatureNames asserts a fixed ordering of returned names, but GetFeatureNames does not document any ordering guarantee (it just forwards whatever order the provider returns). This can make the test flaky or overly constraining for alternative providers; consider sorting both slices or comparing as sets instead of by index.

Copilot uses AI. Check for mistakes.
var fm struct {
FeatureFlags []FeatureFlag `json:"feature_flags"`
}
json.Unmarshal([]byte(jsonData), &fm)
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of json.Unmarshal is ignored here. If the JSON is malformed (or the schema changes), the test will fail later with a less actionable error; capture and assert the unmarshal error so failures point to the real cause.

Suggested change
json.Unmarshal([]byte(jsonData), &fm)
if err := json.Unmarshal([]byte(jsonData), &fm); err != nil {
t.Fatalf("Failed to unmarshal test JSON: %v", err)
}

Copilot uses AI. Check for mistakes.
var fm struct {
FeatureFlags []FeatureFlag `json:"feature_flags"`
}
json.Unmarshal([]byte(jsonData), &fm)
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of json.Unmarshal is ignored here. Please check and fail the test on unmarshal error to avoid masking the root cause of failures.

Suggested change
json.Unmarshal([]byte(jsonData), &fm)
if err := json.Unmarshal([]byte(jsonData), &fm); err != nil {
t.Fatalf("Failed to unmarshal test JSON: %v", err)
}

Copilot uses AI. Check for mistakes.
Comment on lines +236 to +243
enabled, _ := fm.IsEnabled("AnyFilterMixed")
if !enabled {
t.Error("Expected feature to be enabled when one filter returns true with RequirementTypeAny")
}
})

t.Run("All filters false", func(t *testing.T) {
enabled, _ := fm.IsEnabled("AnyFilterAllFalse")
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This subtest ignores the error returned from IsEnabled. If evaluation unexpectedly errors (e.g., validation or filter issues), the assertion may report an incorrect cause; assert err == nil before checking enabled.

Suggested change
enabled, _ := fm.IsEnabled("AnyFilterMixed")
if !enabled {
t.Error("Expected feature to be enabled when one filter returns true with RequirementTypeAny")
}
})
t.Run("All filters false", func(t *testing.T) {
enabled, _ := fm.IsEnabled("AnyFilterAllFalse")
enabled, err := fm.IsEnabled("AnyFilterMixed")
if err != nil {
t.Fatalf("Expected no error evaluating feature AnyFilterMixed: %v", err)
}
if !enabled {
t.Error("Expected feature to be enabled when one filter returns true with RequirementTypeAny")
}
})
t.Run("All filters false", func(t *testing.T) {
enabled, err := fm.IsEnabled("AnyFilterAllFalse")
if err != nil {
t.Fatalf("Expected no error evaluating feature AnyFilterAllFalse: %v", err)
}

Copilot uses AI. Check for mistakes.
Comment on lines +236 to +243
enabled, _ := fm.IsEnabled("AnyFilterMixed")
if !enabled {
t.Error("Expected feature to be enabled when one filter returns true with RequirementTypeAny")
}
})

t.Run("All filters false", func(t *testing.T) {
enabled, _ := fm.IsEnabled("AnyFilterAllFalse")
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This subtest ignores the error returned from IsEnabled. Please assert the error is nil before asserting on the enabled state.

Suggested change
enabled, _ := fm.IsEnabled("AnyFilterMixed")
if !enabled {
t.Error("Expected feature to be enabled when one filter returns true with RequirementTypeAny")
}
})
t.Run("All filters false", func(t *testing.T) {
enabled, _ := fm.IsEnabled("AnyFilterAllFalse")
enabled, err := fm.IsEnabled("AnyFilterMixed")
if err != nil {
t.Fatalf("Expected no error from IsEnabled: %v", err)
}
if !enabled {
t.Error("Expected feature to be enabled when one filter returns true with RequirementTypeAny")
}
})
t.Run("All filters false", func(t *testing.T) {
enabled, err := fm.IsEnabled("AnyFilterAllFalse")
if err != nil {
t.Fatalf("Expected no error from IsEnabled: %v", err)
}

Copilot uses AI. Check for mistakes.
@linglingye001 linglingye001 merged commit 6e9c646 into release/v1.1.2 Apr 16, 2026
17 checks passed
@linglingye001 linglingye001 deleted the linglingye/feature-manager-tests branch April 16, 2026 07:48
linglingye001 added a commit that referenced this pull request Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants