From 7d92aa19717efbc0d35699550741827c243f86e8 Mon Sep 17 00:00:00 2001 From: zees-dev <63374656+zees-dev@users.noreply.github.com> Date: Mon, 15 Nov 2021 12:00:25 +1300 Subject: [PATCH] Unit tests for `enableFeaturesFromFlags` function (#6063) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * - exporting BoolPairs CLI func - added tests for enableFeaturesFromFlags function * Add a test that uses a feature flag to add change the outcome of code - and test persistence, as that's the current implementation Signed-off-by: Sven Dowideit * Minor comment updates Co-authored-by: Sven Dowideit Co-authored-by: Stéphane Busso --- api/cli/cli.go | 2 +- api/cli/pairlistbool.go | 2 +- api/cmd/portainer/main.go | 2 +- api/cmd/portainer/main_test.go | 114 +++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 api/cmd/portainer/main_test.go diff --git a/api/cli/cli.go b/api/cli/cli.go index cc1988860..c2d931c3b 100644 --- a/api/cli/cli.go +++ b/api/cli/cli.go @@ -36,7 +36,7 @@ func (*Service) ParseFlags(version string) (*portainer.CLIFlags, error) { Assets: kingpin.Flag("assets", "Path to the assets").Default(defaultAssetsDirectory).Short('a').String(), Data: kingpin.Flag("data", "Path to the folder where the data is stored").Default(defaultDataDirectory).Short('d').String(), EndpointURL: kingpin.Flag("host", "Environment URL").Short('H').String(), - FeatureFlags: boolPairs(kingpin.Flag("feat", "List of feature flags").Hidden()), + FeatureFlags: BoolPairs(kingpin.Flag("feat", "List of feature flags").Hidden()), EnableEdgeComputeFeatures: kingpin.Flag("edge-compute", "Enable Edge Compute features").Bool(), NoAnalytics: kingpin.Flag("no-analytics", "Disable Analytics in app (deprecated)").Bool(), TLS: kingpin.Flag("tlsverify", "TLS support").Default(defaultTLS).Bool(), diff --git a/api/cli/pairlistbool.go b/api/cli/pairlistbool.go index e1411de6b..2c0949b6d 100644 --- a/api/cli/pairlistbool.go +++ b/api/cli/pairlistbool.go @@ -38,7 +38,7 @@ func (l *pairListBool) IsCumulative() bool { return true } -func boolPairs(s kingpin.Settings) (target *[]portainer.Pair) { +func BoolPairs(s kingpin.Settings) (target *[]portainer.Pair) { target = new([]portainer.Pair) s.SetValue((*pairListBool)(target)) return diff --git a/api/cmd/portainer/main.go b/api/cmd/portainer/main.go index 52b4c2080..60214e3aa 100644 --- a/api/cmd/portainer/main.go +++ b/api/cmd/portainer/main.go @@ -241,7 +241,7 @@ func updateSettingsFromFlags(dataStore portainer.DataStore, flags *portainer.CLI // enableFeaturesFromFlags turns on or off feature flags // e.g. portainer --feat open-amt --feat fdo=true ... (defaults to true) -// note, settins persisted to the DB. To turn off --feat open-amt=false +// note, settings are persisted to the DB. To turn off `--feat open-amt=false` func enableFeaturesFromFlags(dataStore portainer.DataStore, flags *portainer.CLIFlags) error { settings, err := dataStore.Settings().Settings() if err != nil { diff --git a/api/cmd/portainer/main_test.go b/api/cmd/portainer/main_test.go new file mode 100644 index 000000000..689e70f08 --- /dev/null +++ b/api/cmd/portainer/main_test.go @@ -0,0 +1,114 @@ +package main + +import ( + "fmt" + "testing" + + portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/api/bolt" + "github.com/portainer/portainer/api/cli" + "github.com/stretchr/testify/assert" + "gopkg.in/alecthomas/kingpin.v2" +) + +type mockKingpinSetting string + +func (m mockKingpinSetting) SetValue(value kingpin.Value) { + value.Set(string(m)) +} + +func Test_enableFeaturesFromFlags(t *testing.T) { + is := assert.New(t) + + store, teardown := bolt.MustNewTestStore(true) + defer teardown() + + tests := []struct { + featureFlag string + isSupported bool + }{ + {"test", false}, + {"openamt", false}, + {"open-amt", true}, + {"oPeN-amT", true}, + {"fdo", true}, + {"FDO", true}, + } + for _, test := range tests { + t.Run(fmt.Sprintf("%s succeeds:%v", test.featureFlag, test.isSupported), func(t *testing.T) { + mockKingpinSetting := mockKingpinSetting(test.featureFlag) + flags := &portainer.CLIFlags{FeatureFlags: cli.BoolPairs(mockKingpinSetting)} + err := enableFeaturesFromFlags(store, flags) + if test.isSupported { + is.NoError(err) + } else { + is.Error(err) + } + }) + } + + t.Run("passes for all supported feature flags", func(t *testing.T) { + for _, flag := range portainer.SupportedFeatureFlags { + mockKingpinSetting := mockKingpinSetting(flag) + flags := &portainer.CLIFlags{FeatureFlags: cli.BoolPairs(mockKingpinSetting)} + err := enableFeaturesFromFlags(store, flags) + is.NoError(err) + } + }) +} + +const FeatTest portainer.Feature = "optional-test" + +func optionalFunc(dataStore portainer.DataStore) string { + + // TODO: this is a code smell - finding out if a feature flag is enabled should not require having access to the store, and asking for a settings obj. + // ideally, the `if` should look more like: + // if featureflags.FlagEnabled(FeatTest) {} + settings, err := dataStore.Settings().Settings() + if err != nil { + return err.Error() + } + + if settings.FeatureFlagSettings[FeatTest] { + return "enabled" + } + return "disabled" +} + +func Test_optionalFeature(t *testing.T) { + portainer.SupportedFeatureFlags = append(portainer.SupportedFeatureFlags, FeatTest) + + is := assert.New(t) + + store, teardown := bolt.MustNewTestStore(true) + defer teardown() + + // Enable the test feature + t.Run(fmt.Sprintf("%s succeeds:%v", FeatTest, true), func(t *testing.T) { + mockKingpinSetting := mockKingpinSetting(FeatTest) + flags := &portainer.CLIFlags{FeatureFlags: cli.BoolPairs(mockKingpinSetting)} + err := enableFeaturesFromFlags(store, flags) + is.NoError(err) + is.Equal("enabled", optionalFunc(store)) + }) + + // Same store, so the feature flag should still be enabled + t.Run(fmt.Sprintf("%s succeeds:%v", FeatTest, true), func(t *testing.T) { + is.Equal("enabled", optionalFunc(store)) + }) + + // disable the test feature + t.Run(fmt.Sprintf("%s succeeds:%v", FeatTest, true), func(t *testing.T) { + mockKingpinSetting := mockKingpinSetting(FeatTest + "=false") + flags := &portainer.CLIFlags{FeatureFlags: cli.BoolPairs(mockKingpinSetting)} + err := enableFeaturesFromFlags(store, flags) + is.NoError(err) + is.Equal("disabled", optionalFunc(store)) + }) + + // Same store, so feature flag should still be disabled + t.Run(fmt.Sprintf("%s succeeds:%v", FeatTest, true), func(t *testing.T) { + is.Equal("disabled", optionalFunc(store)) + }) + +}