diff --git a/api/turing/api/deployment_controller.go b/api/turing/api/deployment_controller.go index 0f6766359..46c4db184 100644 --- a/api/turing/api/deployment_controller.go +++ b/api/turing/api/deployment_controller.go @@ -5,6 +5,8 @@ import ( "errors" "strings" + "github.com/gojek/turing/engines/experiment/manager" + merlin "github.com/gojek/merlin/client" mlp "github.com/gojek/mlp/api/client" "github.com/gojek/turing/api/turing/models" @@ -189,9 +191,10 @@ func (c RouterDeploymentController) deployRouterVersion( expSvc := c.BaseController.AppContext.ExperimentsService if routerVersion.ExperimentEngine.Type != models.ExperimentEngineTypeNop { - if expSvc.IsStandardExperimentManager(string(routerVersion.ExperimentEngine.Type)) { + + if expSvc.IsStandardExperimentManager(routerVersion.ExperimentEngine.Type) { // Convert the config to the standard type - expConfig, err := expSvc.GetStandardExperimentConfig(routerVersion.ExperimentEngine.Config) + expConfig, err := manager.ParseStandardExperimentConfig(routerVersion.ExperimentEngine.Config) if err != nil { return "", c.updateRouterVersionStatusToFailed(err, routerVersion) } @@ -206,7 +209,7 @@ func (c RouterDeploymentController) deployRouterVersion( // Get the deployable Router Config for the experiment experimentConfig, err = c.ExperimentsService.GetExperimentRunnerConfig( - string(routerVersion.ExperimentEngine.Type), + routerVersion.ExperimentEngine.Type, routerVersion.ExperimentEngine.Config, ) if err != nil { diff --git a/api/turing/api/deployment_controller_test.go b/api/turing/api/deployment_controller_test.go index 5667e4d9d..103e2caee 100644 --- a/api/turing/api/deployment_controller_test.go +++ b/api/turing/api/deployment_controller_test.go @@ -15,7 +15,6 @@ import ( "github.com/gojek/turing/api/turing/models" "github.com/gojek/turing/api/turing/service/mocks" "github.com/gojek/turing/api/turing/utils" - "github.com/gojek/turing/engines/experiment/manager" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -40,14 +39,12 @@ func TestDeployVersionSuccess(t *testing.T) { nopExpCfg := &models.ExperimentEngine{ Type: "nop", } - expCfg := &manager.TuringExperimentConfig{ - Client: manager.Client{ - ID: "1", - Passkey: "xp-passkey", - }, - } + expCfg := json.RawMessage(`{"client": {"id": "1", "passkey": "test-passkey"}}`) + + testEngineType := "test-manager" + expEnabledCfg := &models.ExperimentEngine{ - Type: "xp", + Type: testEngineType, Config: expCfg, } @@ -111,8 +108,8 @@ func TestDeployVersionSuccess(t *testing.T) { ExperimentEngine: expEnabledCfg, Status: "deployed", }, - expCfg: json.RawMessage([]byte(`{"engine": "xp"}`)), - decryptedPasskey: "xp-passkey-dec", + expCfg: json.RawMessage(`{"engine": "test"}`), + decryptedPasskey: "test-passkey-dec", }, } @@ -130,13 +127,12 @@ func TestDeployVersionSuccess(t *testing.T) { es.On("Save", mock.Anything).Return(nil) cs := &mocks.CryptoService{} - cs.On("Decrypt", "xp-passkey").Return("xp-passkey-dec", nil) + cs.On("Decrypt", "test-passkey").Return("test-passkey-dec", nil) exps := &mocks.ExperimentsService{} exps.On("IsStandardExperimentManager", "nop").Return(false) exps.On("IsStandardExperimentManager", mock.Anything).Return(true) - exps.On("GetStandardExperimentConfig", expCfg).Return(*expCfg, nil) - exps.On("GetExperimentRunnerConfig", "xp", expCfg).Return(json.RawMessage([]byte(`{"engine": "xp"}`)), nil) + exps.On("GetExperimentRunnerConfig", testEngineType, expCfg).Return(json.RawMessage(`{"engine": "test"}`), nil) // Run tests and validate for name, data := range tests { diff --git a/api/turing/api/request/request.go b/api/turing/api/request/request.go index b233bd05d..315b9cead 100644 --- a/api/turing/api/request/request.go +++ b/api/turing/api/request/request.go @@ -1,6 +1,7 @@ package request import ( + "encoding/json" "errors" "fmt" @@ -36,8 +37,8 @@ type RouterConfig struct { // ExperimentEngineConfig defines the experiment engine config type ExperimentEngineConfig struct { - Type string `json:"type" validate:"required,oneof=litmus nop xp"` - Config interface{} `json:"config,omitempty" validate:"-"` // Skip validate to invoke custom validation + Type string `json:"type" validate:"required"` + Config json.RawMessage `json:"config,omitempty" validate:"-"` // Skip validate to invoke custom validation } // LogConfig defines the logging configs @@ -124,7 +125,7 @@ func (r CreateOrUpdateRouterRequest) BuildRouterVersion( DefaultRouteID: r.Config.DefaultRouteID, TrafficRules: r.Config.TrafficRules, ExperimentEngine: &models.ExperimentEngine{ - Type: models.ExperimentEngineType(r.Config.ExperimentEngine.Type), + Type: r.Config.ExperimentEngine.Type, }, ResourceRequest: r.Config.ResourceRequest, Timeout: r.Config.Timeout, @@ -133,7 +134,7 @@ func (r CreateOrUpdateRouterRequest) BuildRouterVersion( CustomMetricsEnabled: defaults.CustomMetricsEnabled, FiberDebugLogEnabled: defaults.FiberDebugLogEnabled, JaegerEnabled: defaults.JaegerEnabled, - ResultLoggerType: models.ResultLogger(r.Config.LogConfig.ResultLoggerType), + ResultLoggerType: r.Config.LogConfig.ResultLoggerType, }, } if r.Config.Enricher != nil { @@ -179,54 +180,45 @@ func (r CreateOrUpdateRouterRequest) BuildExperimentEngineConfig( router *models.Router, cryptoSvc service.CryptoService, expSvc service.ExperimentsService, -) (interface{}, error) { +) (json.RawMessage, error) { rawExpConfig := r.Config.ExperimentEngine.Config // Handle missing passkey / encrypt it in Standard experiment config if expSvc.IsStandardExperimentManager(r.Config.ExperimentEngine.Type) { // Convert the new config to the standard type - expConfig, err := expSvc.GetStandardExperimentConfig(rawExpConfig) + expConfig, err := manager.ParseStandardExperimentConfig(rawExpConfig) if err != nil { return nil, fmt.Errorf("Cannot parse standard experiment config: %v", err) } - clientPasskey := expConfig.Client.Passkey - if clientPasskey == "" { + if expConfig.Client.Passkey == "" { // Extract existing router version config if router.CurrRouterVersion != nil && - string(router.CurrRouterVersion.ExperimentEngine.Type) == r.Config.ExperimentEngine.Type { - currVerExpConfig, err := expSvc.GetStandardExperimentConfig(router.CurrRouterVersion.ExperimentEngine.Config) + router.CurrRouterVersion.ExperimentEngine.Type == r.Config.ExperimentEngine.Type { + currVerExpConfig, err := manager.ParseStandardExperimentConfig(router.CurrRouterVersion.ExperimentEngine.Config) if err != nil { return nil, fmt.Errorf("Error parsing existing experiment config: %v", err) } if expConfig.Client.Username == currVerExpConfig.Client.Username { // Copy the passkey - clientPasskey = currVerExpConfig.Client.Passkey + expConfig.Client.Passkey = currVerExpConfig.Client.Passkey } } // If the passkey is still empty, we cannot proceed - if clientPasskey == "" { + if expConfig.Client.Passkey == "" { return nil, errors.New("Passkey must be configured") } } else { // Passkey has been supplied, encrypt it var err error - clientPasskey, err = cryptoSvc.Encrypt(clientPasskey) + expConfig.Client.Passkey, err = cryptoSvc.Encrypt(expConfig.Client.Passkey) if err != nil { return nil, fmt.Errorf("Passkey could not be encrypted: %s", err.Error()) } } - // Build Experiment engine config - return &manager.TuringExperimentConfig{ - Client: manager.Client{ - ID: expConfig.Client.ID, - Username: expConfig.Client.Username, - Passkey: clientPasskey, - }, - Experiments: expConfig.Experiments, - Variables: expConfig.Variables, - }, nil + // Marshal Experiment engine config + return json.Marshal(expConfig) } // Custom experiment manager config, return as is. diff --git a/api/turing/api/request/request_test.go b/api/turing/api/request/request_test.go index a618266ab..6df01721f 100644 --- a/api/turing/api/request/request_test.go +++ b/api/turing/api/request/request_test.go @@ -3,6 +3,7 @@ package request import ( + "encoding/json" "errors" "testing" @@ -19,48 +20,51 @@ import ( "github.com/gojek/turing/engines/experiment/pkg/request" ) -var expEngineConfig = manager.TuringExperimentConfig{ - Client: manager.Client{ - ID: "1", - Username: "client", - Passkey: "dummy_passkey", - }, - Experiments: []manager.Experiment{ - { - ID: "2", - Name: "test-exp", +func makeTuringExperimentConfig(clientPasskey string) json.RawMessage { + expEngineConfig, _ := json.Marshal(manager.TuringExperimentConfig{ + Client: manager.Client{ + ID: "1", + Username: "client", + Passkey: clientPasskey, }, - }, - Variables: manager.Variables{ - ClientVariables: []manager.Variable{ + Experiments: []manager.Experiment{ { - Name: "app_version", - Required: false, + ID: "2", + Name: "test-exp", }, }, - ExperimentVariables: map[string][]manager.Variable{ - "2": { + Variables: manager.Variables{ + ClientVariables: []manager.Variable{ { - Name: "customer", - Required: true, + Name: "app_version", + Required: false, }, }, - }, - Config: []manager.VariableConfig{ - { - Name: "customer", - Required: true, - Field: "customer_id", - FieldSource: request.HeaderFieldSource, + ExperimentVariables: map[string][]manager.Variable{ + "2": { + { + Name: "customer", + Required: true, + }, + }, }, - { - Name: "app_version", - Required: false, - Field: "test_field", - FieldSource: request.HeaderFieldSource, + Config: []manager.VariableConfig{ + { + Name: "customer", + Required: true, + Field: "customer_id", + FieldSource: request.HeaderFieldSource, + }, + { + Name: "app_version", + Required: false, + Field: "test_field", + FieldSource: request.HeaderFieldSource, + }, }, }, - }, + }) + return expEngineConfig } var createOrUpdateRequest = CreateOrUpdateRouterRequest{ @@ -77,8 +81,8 @@ var createOrUpdateRequest = CreateOrUpdateRouterRequest{ }, DefaultRouteID: "default", ExperimentEngine: &ExperimentEngineConfig{ - Type: "litmus", - Config: &expEngineConfig, + Type: "standard", + Config: makeTuringExperimentConfig("dummy_passkey"), }, ResourceRequest: &models.ResourceRequest{ MinReplica: 0, @@ -160,7 +164,7 @@ func TestRequestBuildRouterVersionWithDefaults(t *testing.T) { Tag: "fluentdtag", }, Experiment: map[string]interface{}{ - "litmus": map[string]interface{}{ + "standard": map[string]interface{}{ "endpoint": "grpc://test", "timeout": "2s", }, @@ -168,6 +172,7 @@ func TestRequestBuildRouterVersionWithDefaults(t *testing.T) { } projectID := models.ID(1) router := createOrUpdateRequest.BuildRouter(projectID) + expected := models.RouterVersion{ Router: router, Status: "pending", @@ -182,50 +187,8 @@ func TestRequestBuildRouterVersionWithDefaults(t *testing.T) { }, DefaultRouteID: "default", ExperimentEngine: &models.ExperimentEngine{ - Type: "litmus", - Config: &manager.TuringExperimentConfig{ - Client: manager.Client{ - ID: "1", - Username: "client", - Passkey: "enc_passkey", - }, - Experiments: []manager.Experiment{ - { - ID: "2", - Name: "test-exp", - }, - }, - Variables: manager.Variables{ - ClientVariables: []manager.Variable{ - { - Name: "app_version", - Required: false, - }, - }, - ExperimentVariables: map[string][]manager.Variable{ - "2": { - { - Name: "customer", - Required: true, - }, - }, - }, - Config: []manager.VariableConfig{ - { - Name: "customer", - Required: true, - Field: "customer_id", - FieldSource: request.HeaderFieldSource, - }, - { - Name: "app_version", - Required: false, - Field: "test_field", - FieldSource: request.HeaderFieldSource, - }, - }, - }, - }, + Type: "standard", + Config: makeTuringExperimentConfig("enc_passkey"), }, ResourceRequest: &models.ResourceRequest{ MinReplica: 0, @@ -292,7 +255,6 @@ func TestRequestBuildRouterVersionWithDefaults(t *testing.T) { // Set up mock Experiment service expSvc := &mocks.ExperimentsService{} expSvc.On("IsStandardExperimentManager", mock.Anything).Return(true) - expSvc.On("GetStandardExperimentConfig", &expEngineConfig).Return(expEngineConfig, nil) got, err := createOrUpdateRequest.BuildRouterVersion(router, &defaults, cryptoSvc, expSvc) tu.FailOnError(t, err) @@ -303,47 +265,28 @@ func TestRequestBuildRouterVersionWithDefaults(t *testing.T) { func TestBuildExperimentEngineConfig(t *testing.T) { // Set up mock Crypto service cs := &mocks.CryptoService{} - cs.On("Encrypt", "xp-passkey-bad").Return("", errors.New("test-encrypt-error")) - cs.On("Encrypt", "xp-passkey").Return("xp-passkey-enc", nil) + cs.On("Encrypt", "passkey-bad"). + Return("", errors.New("test-encrypt-error")) + cs.On("Encrypt", "passkey"). + Return("passkey-enc", nil) - // Set up mock Experiment service - cfgWithPasskey := &manager.TuringExperimentConfig{ - Client: manager.Client{ - Username: "client-name", - Passkey: "xp-passkey", - }, - } - cfgWithoutPasskey := &manager.TuringExperimentConfig{ - Client: manager.Client{ - Username: "client-name", - }, - } - cfgWithBadPasskey := &manager.TuringExperimentConfig{ - Client: manager.Client{ - Username: "client-name", - Passkey: "xp-passkey-bad", - }, - } es := &mocks.ExperimentsService{} - es.On("IsStandardExperimentManager", "litmus").Return(true) - es.On("IsStandardExperimentManager", "xp").Return(false) - es.On("GetStandardExperimentConfig", cfgWithPasskey).Return(*cfgWithPasskey, nil) - es.On("GetStandardExperimentConfig", cfgWithoutPasskey).Return(*cfgWithoutPasskey, nil) - es.On("GetStandardExperimentConfig", cfgWithBadPasskey).Return(*cfgWithBadPasskey, nil) + es.On("IsStandardExperimentManager", "standard-manager").Return(true) + es.On("IsStandardExperimentManager", "custom-manager").Return(false) // Define tests tests := map[string]struct { req CreateOrUpdateRouterRequest router *models.Router - expected interface{} + expected json.RawMessage err string }{ "failure | std engine | missing curr version passkey": { req: CreateOrUpdateRouterRequest{ Config: &RouterConfig{ ExperimentEngine: &ExperimentEngineConfig{ - Type: "litmus", - Config: cfgWithoutPasskey, + Type: "standard-manager", + Config: json.RawMessage(`{"client": {"username": "client-name"}}`), }, }, }, @@ -354,8 +297,8 @@ func TestBuildExperimentEngineConfig(t *testing.T) { req: CreateOrUpdateRouterRequest{ Config: &RouterConfig{ ExperimentEngine: &ExperimentEngineConfig{ - Type: "litmus", - Config: cfgWithBadPasskey, + Type: "standard-manager", + Config: json.RawMessage(`{"client": {"username": "client-name", "passkey": "passkey-bad"}}`), }, }, }, @@ -366,49 +309,44 @@ func TestBuildExperimentEngineConfig(t *testing.T) { req: CreateOrUpdateRouterRequest{ Config: &RouterConfig{ ExperimentEngine: &ExperimentEngineConfig{ - Type: "litmus", - Config: cfgWithoutPasskey, + Type: "standard-manager", + Config: json.RawMessage(`{"client": {"username": "client-name"}}`), }, }, }, router: &models.Router{ CurrRouterVersion: &models.RouterVersion{ ExperimentEngine: &models.ExperimentEngine{ - Type: "litmus", - Config: cfgWithPasskey, + Type: "standard-manager", + Config: json.RawMessage(`{"client": {"username": "client-name", "passkey": "passkey"}}`), }, }, }, - expected: cfgWithPasskey, + expected: json.RawMessage(`{"client":{"id":"","username":"client-name","passkey":"passkey"},"experiments":null,"variables":{"client_variables":null,"experiment_variables":null,"config":null}}`), }, "success | std engine | use new passkey": { req: CreateOrUpdateRouterRequest{ Config: &RouterConfig{ ExperimentEngine: &ExperimentEngineConfig{ - Type: "litmus", - Config: cfgWithPasskey, + Type: "standard-manager", + Config: json.RawMessage(`{"client": {"username": "client-name", "passkey": "passkey"}}`), }, }, }, - router: &models.Router{}, - expected: &manager.TuringExperimentConfig{ - Client: manager.Client{ - Username: "client-name", - Passkey: "xp-passkey-enc", - }, - }, + router: &models.Router{}, + expected: json.RawMessage(`{"client":{"id":"","username":"client-name","passkey":"passkey-enc"},"experiments":null,"variables":{"client_variables":null,"experiment_variables":null,"config":null}}`), }, "success | custom engine": { req: CreateOrUpdateRouterRequest{ Config: &RouterConfig{ ExperimentEngine: &ExperimentEngineConfig{ - Type: "xp", - Config: []int{1, 2}, + Type: "custom-manager", + Config: json.RawMessage("[1, 2]"), }, }, }, router: &models.Router{}, - expected: []int{1, 2}, + expected: json.RawMessage("[1, 2]"), }, } diff --git a/api/turing/models/experiment_engine.go b/api/turing/models/experiment_engine.go index f32020965..5c313bc06 100644 --- a/api/turing/models/experiment_engine.go +++ b/api/turing/models/experiment_engine.go @@ -6,25 +6,21 @@ import ( "errors" ) -// EngineType is used to capture the types of the supported experimentation engines -// Need to expose this value in turing-router -// engines/router/missionctl/experiment/experiment.go -type ExperimentEngineType string - const ( - ExperimentEngineTypeNop ExperimentEngineType = "nop" - ExperimentEngineTypeLitmus ExperimentEngineType = "litmus" - ExperimentEngineTypeXp ExperimentEngineType = "xp" + ExperimentEngineTypeNop string = "nop" + ExperimentEngineTypeLitmus string = "litmus" + ExperimentEngineTypeXp string = "xp" ) // ExperimentEngine contains the type and configuration for the // Experiment engine powering the router. type ExperimentEngine struct { - // Type of Experiment Engine. Currently supports "litmus", "xp" and "nop". - Type ExperimentEngineType `json:"type"` + // Type of Experiment Engine + Type string `json:"type"` // Config contains the configs for the selected experiment engine (other than "nop"). - // For standard experiment engine managers, the config can be cast into TuringExperimentConfig type. - Config interface{} `json:"config,omitempty"` + // For standard experiment engine managers, the config can be unmarshalled into + // manager.TuringExperimentConfig type. + Config json.RawMessage `json:"config,omitempty"` } func (eec ExperimentEngine) Value() (driver.Value, error) { diff --git a/api/turing/models/experiment_engine_test.go b/api/turing/models/experiment_engine_test.go index 07cae65e5..0daaaa2c3 100644 --- a/api/turing/models/experiment_engine_test.go +++ b/api/turing/models/experiment_engine_test.go @@ -3,109 +3,9 @@ package models import ( "testing" - "github.com/gojek/turing/engines/experiment/manager" - "github.com/gojek/turing/engines/experiment/pkg/request" "github.com/stretchr/testify/assert" ) -func TestExperimentEngineValue(t *testing.T) { - tests := map[string]struct { - expEngine ExperimentEngine - expected string - }{ - "nop": { - expEngine: ExperimentEngine{ - Type: ExperimentEngineTypeNop, - }, - expected: string(`{ - "type": "nop" - }`), - }, - "litmus": { - expEngine: ExperimentEngine{ - Type: ExperimentEngineTypeLitmus, - Config: &manager.TuringExperimentConfig{ - Client: manager.Client{ - ID: "1", - Username: "c1", - Passkey: "encrypted", - }, - Experiments: []manager.Experiment{}, - Variables: manager.Variables{ - ClientVariables: []manager.Variable{ - { - Name: "var-1", - Required: true, - Type: manager.UnsupportedVariableType, - }, - }, - ExperimentVariables: map[string][]manager.Variable{}, - Config: []manager.VariableConfig{ - { - Name: "var-1", - Required: true, - Field: "field1", - FieldSource: request.HeaderFieldSource, - }, - }, - }, - }, - }, - expected: string(`{ - "type": "litmus", - "config": { - "client": { - "id": "1", - "username": "c1", - "passkey": "encrypted" - }, - "experiments": [], - "variables": { - "client_variables": [ - { - "name": "var-1", - "required": true, - "type": "unsupported" - } - ], - "experiment_variables": {}, - "config": [ - { - "name": "var-1", - "required": true, - "field": "field1", - "field_source": "header" - } - ] - } - } - }`), - }, - "xp": { - expEngine: ExperimentEngine{ - Type: ExperimentEngineTypeXp, - Config: []int{1, 2}, - }, - expected: string(`{ - "type": "xp", - "config": [1,2] - }`), - }, - } - - for name, data := range tests { - t.Run(name, func(t *testing.T) { - value, err := data.expEngine.Value() - // Convert to string for comparison - byteValue, ok := value.([]byte) - assert.True(t, ok) - // Validate - assert.NoError(t, err) - assert.JSONEq(t, data.expected, string(byteValue)) - }) - } -} - func TestExperimentEngineScan(t *testing.T) { tests := map[string]struct { value interface{} diff --git a/api/turing/models/traffic_rules_test.go b/api/turing/models/traffic_rules_test.go index 917fcb75c..4bd07e963 100644 --- a/api/turing/models/traffic_rules_test.go +++ b/api/turing/models/traffic_rules_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/gojek/turing/api/turing/models" + "github.com/gojek/turing/engines/experiment/pkg/request" "github.com/gojek/turing/engines/router" "github.com/stretchr/testify/require" ) @@ -74,7 +75,7 @@ func TestTrafficRules_Scan(t *testing.T) { &models.TrafficRule{ Conditions: []*router.TrafficRuleCondition{ { - FieldSource: "header", + FieldSource: request.HeaderFieldSource, Field: "X-Region", Operator: router.InConditionOperator, Values: []string{ @@ -107,7 +108,7 @@ func TestTrafficRules_Scan(t *testing.T) { serialized: 100, expectedError: "type assertion to []byte failed", }, - "failure | unknown field_source": { + "success | unknown field_source": { serialized: []byte(` [{ "conditions": [{ @@ -119,7 +120,21 @@ func TestTrafficRules_Scan(t *testing.T) { "routes": [] }] `), - expectedError: "Unknown field source unknown", + trafficRules: models.TrafficRules{ + &models.TrafficRule{ + Conditions: []*router.TrafficRuleCondition{ + { + FieldSource: "unknown", + Field: "X-Region", + Operator: router.InConditionOperator, + Values: []string{ + "region-a", "region-b", + }, + }, + }, + Routes: []string{}, + }, + }, }, } diff --git a/api/turing/service/experiment_service.go b/api/turing/service/experiment_service.go index 2c0b6c651..7aec2425d 100644 --- a/api/turing/service/experiment_service.go +++ b/api/turing/service/experiment_service.go @@ -24,8 +24,6 @@ const ( type ExperimentsService interface { // IsStandardExperimentManager checks if the experiment manager is of the standard type IsStandardExperimentManager(engine string) bool - // GetStandardExperimentConfig converts the given generic config into the standard type, if possible - GetStandardExperimentConfig(config interface{}) (manager.TuringExperimentConfig, error) // ListEngines returns a list of the experiment engines available ListEngines() []manager.Engine // ListClients returns a list of the clients registered on the given experiment engine @@ -37,10 +35,10 @@ type ExperimentsService interface { // for the given clientID and/or experiments ListVariables(engine string, clientID string, experimentIDs []string) (manager.Variables, error) // ValidateExperimentConfig validates the given experiment config for completeness - ValidateExperimentConfig(engine string, cfg interface{}) error + ValidateExperimentConfig(engine string, cfg json.RawMessage) error // GetExperimentRunnerConfig converts the given experiment config compatible with the Experiment Manager // into the format compatible with the ExperimentRunner - GetExperimentRunnerConfig(engine string, cfg interface{}) (json.RawMessage, error) + GetExperimentRunnerConfig(engine string, cfg json.RawMessage) (json.RawMessage, error) } type experimentsService struct { @@ -88,10 +86,15 @@ func NewExperimentsService(managerConfig map[string]config.EngineConfig) (Experi // Populate the cache with the Clients / Experiments info from Standard Engines for expEngine, expManager := range svc.experimentManagers { - engineInfo := expManager.GetEngineInfo() + engineInfo, err := expManager.GetEngineInfo() + if err != nil { + logger.Warnf("failed to retrieve info for engine %s: %v", expEngine, err) + continue + } + if engineInfo.Type == manager.StandardExperimentManagerType { if engineInfo.StandardExperimentManagerConfig == nil { - return nil, fmt.Errorf("Standard Experiment Manager config missing for engine %s", engineInfo.Name) + return nil, fmt.Errorf("Standard Experiment Manager config missing for engine %s", expEngine) } if engineInfo.StandardExperimentManagerConfig.ClientSelectionEnabled { _, err := svc.ListClients(expEngine) @@ -115,18 +118,18 @@ func (es *experimentsService) IsStandardExperimentManager(engine string) bool { if err != nil { return false } - return expManager.GetEngineInfo().Type == manager.StandardExperimentManagerType -} - -func (*experimentsService) GetStandardExperimentConfig(config interface{}) (manager.TuringExperimentConfig, error) { - return manager.GetStandardExperimentConfig(config) + return manager.IsStandardExperimentManager(expManager) } func (es *experimentsService) ListEngines() []manager.Engine { engines := []manager.Engine{} for _, expManager := range es.experimentManagers { - engines = append(engines, expManager.GetEngineInfo()) + engineInfo, err := expManager.GetEngineInfo() + if err == nil { + engines = append(engines, engineInfo) + } + } return engines } @@ -199,24 +202,23 @@ func (es *experimentsService) ListVariables( }, nil } -func (es *experimentsService) ValidateExperimentConfig(engine string, cfg interface{}) error { +func (es *experimentsService) ValidateExperimentConfig(engine string, cfg json.RawMessage) error { // Get experiment manager expManager, err := es.getExperimentManager(engine) if err != nil { return err } - - return manager.ValidateExperimentConfig(expManager, cfg) + return expManager.ValidateExperimentConfig(cfg) } -func (es *experimentsService) GetExperimentRunnerConfig(engine string, cfg interface{}) (json.RawMessage, error) { +func (es *experimentsService) GetExperimentRunnerConfig(engine string, cfg json.RawMessage) (json.RawMessage, error) { // Get experiment manager expManager, err := es.getExperimentManager(engine) if err != nil { return json.RawMessage{}, err } - return manager.GetExperimentRunnerConfig(expManager, cfg) + return expManager.GetExperimentRunnerConfig(cfg) } func (es *experimentsService) getExperimentManager( diff --git a/api/turing/service/experiment_service_test.go b/api/turing/service/experiment_service_test.go index 6aac79305..e6f174786 100644 --- a/api/turing/service/experiment_service_test.go +++ b/api/turing/service/experiment_service_test.go @@ -3,6 +3,7 @@ package service import ( "encoding/json" "errors" + "fmt" "sort" "testing" "time" @@ -10,6 +11,7 @@ import ( "github.com/patrickmn/go-cache" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" tu "github.com/gojek/turing/api/turing/internal/testutils" "github.com/gojek/turing/engines/experiment/manager" @@ -22,16 +24,16 @@ var customExperimentManagerConfig = manager.Engine{Type: manager.CustomExperimen func TestListEngines(t *testing.T) { // Set up mock Experiment Managers - litmusEngineInfo := manager.Engine{ - Name: "Litmus", + standardEngineInfo := manager.Engine{ + Name: "standard-engine", Type: manager.StandardExperimentManagerType, StandardExperimentManagerConfig: &manager.StandardExperimentManagerConfig{ ClientSelectionEnabled: true, ExperimentSelectionEnabled: true, }, } - xpEngineInfo := manager.Engine{ - Name: "XP", + customEngineInfo := manager.Engine{ + Name: "custom-engine", Type: manager.StandardExperimentManagerType, StandardExperimentManagerConfig: &manager.StandardExperimentManagerConfig{ ClientSelectionEnabled: false, @@ -39,14 +41,14 @@ func TestListEngines(t *testing.T) { }, } expMgr1 := &mocks.ExperimentManager{} - expMgr1.On("GetEngineInfo").Return(litmusEngineInfo) + expMgr1.On("GetEngineInfo").Return(standardEngineInfo, nil) expMgr2 := &mocks.ExperimentManager{} - expMgr2.On("GetEngineInfo").Return(xpEngineInfo) + expMgr2.On("GetEngineInfo").Return(customEngineInfo, nil) // Create the experiment managers map and the experiment service experimentManagers := make(map[string]manager.ExperimentManager) - experimentManagers["litmus"] = expMgr1 - experimentManagers["xp"] = expMgr2 + experimentManagers["standard-engine"] = expMgr1 + experimentManagers["custom-engine"] = expMgr2 svc := &experimentsService{ experimentManagers: experimentManagers, cache: cache.New(time.Second, time.Second), @@ -58,7 +60,7 @@ func TestListEngines(t *testing.T) { sort.SliceStable(response, func(i, j int) bool { return response[i].Name < response[j].Name }) - assert.Equal(t, []manager.Engine{litmusEngineInfo, xpEngineInfo}, response) + assert.Equal(t, []manager.Engine{customEngineInfo, standardEngineInfo}, response) } func TestListClients(t *testing.T) { @@ -70,15 +72,16 @@ func TestListClients(t *testing.T) { } // Set up mock Experiment Managers expMgrSuccess := &mocks.StandardExperimentManager{} - expMgrSuccess.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrSuccess.On("IsCacheEnabled").Return(true) + expMgrSuccess.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrSuccess.On("IsCacheEnabled").Return(true, nil) expMgrSuccess.On("ListClients").Return(clients, nil) expMgrFailure := &mocks.StandardExperimentManager{} - expMgrFailure.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrFailure.On("IsCacheEnabled").Return(true) + expMgrFailure.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrFailure.On("IsCacheEnabled").Return(true, nil) expMgrFailure.On("ListClients").Return([]manager.Client{}, errors.New("List clients error")) + expManagerName := "exp-engine-000" // Define tests tests := map[string]struct { expMgr manager.ExperimentManager @@ -101,7 +104,7 @@ func TestListClients(t *testing.T) { expMgr: expMgrSuccess, useBadCache: true, expected: []manager.Client{}, - err: "Malformed clients info found in the cache for engine litmus", + err: fmt.Sprintf("Malformed clients info found in the cache for engine %s", expManagerName), }, } @@ -111,17 +114,17 @@ func TestListClients(t *testing.T) { // Create experiment service cacheObj := cache.New(time.Second*2, time.Second*2) if data.useBadCache { - cacheObj.Set("engine:litmus:clients", "test", cache.DefaultExpiration) + cacheObj.Set(fmt.Sprintf("engine:%s:clients", expManagerName), "test", cache.DefaultExpiration) } svc := &experimentsService{ experimentManagers: map[string]manager.ExperimentManager{ - "litmus": data.expMgr, + expManagerName: data.expMgr, }, cache: cacheObj, } // Run and Validate - response, err := svc.ListClients("litmus") + response, err := svc.ListClients(expManagerName) assert.Equal(t, data.expected, response) if data.err != "" { tu.FailOnNil(t, err) @@ -130,7 +133,7 @@ func TestListClients(t *testing.T) { // Access cache if data.err == "" { - response, err := svc.ListClients("litmus") + response, err := svc.ListClients(expManagerName) assert.Equal(t, data.expected, response) assert.NoError(t, err) } @@ -153,20 +156,20 @@ func TestListExperiments(t *testing.T) { } // Set up mock Experiment Managers expMgrSuccess := &mocks.StandardExperimentManager{} - expMgrSuccess.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrSuccess.On("IsCacheEnabled").Return(true) + expMgrSuccess.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrSuccess.On("IsCacheEnabled").Return(true, nil) expMgrSuccess.On("ListClients").Return(clients, nil) expMgrSuccess.On("ListExperiments").Return(experiments, nil) expMgrSuccess.On("ListExperimentsForClient", client).Return(experiments, nil) expMgrFailure1 := &mocks.StandardExperimentManager{} - expMgrFailure1.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrFailure1.On("IsCacheEnabled").Return(true) + expMgrFailure1.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrFailure1.On("IsCacheEnabled").Return(true, nil) expMgrFailure1.On("ListClients").Return([]manager.Client{}, errors.New("List clients error")) expMgrFailure2 := &mocks.StandardExperimentManager{} - expMgrFailure2.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrFailure2.On("IsCacheEnabled").Return(true) + expMgrFailure2.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrFailure2.On("IsCacheEnabled").Return(true, nil) expMgrFailure2.On("ListClients").Return(clients, nil) expMgrFailure2.On("ListExperimentsForClient", client). Return([]manager.Experiment{}, errors.New("List experiments error")) @@ -213,23 +216,28 @@ func TestListExperiments(t *testing.T) { }, } + expManagerName := "exp-engine-001" // Run tests for name, data := range tests { t.Run(name, func(t *testing.T) { // Create experiment service cacheObj := cache.New(time.Second*2, time.Second*2) if data.useBadCache { - cacheObj.Set("engine:litmus:clients:1:experiments", "test", cache.DefaultExpiration) + cacheObj.Set( + fmt.Sprintf("engine:%s:clients:1:experiments", expManagerName), + "test", + cache.DefaultExpiration, + ) } svc := &experimentsService{ experimentManagers: map[string]manager.ExperimentManager{ - "litmus": data.expMgr, + expManagerName: data.expMgr, }, cache: cacheObj, } // Run and Validate - response, err := svc.ListExperiments("litmus", data.clientID) + response, err := svc.ListExperiments(expManagerName, data.clientID) assert.Equal(t, data.expected, response) if data.err != "" { tu.FailOnNil(t, err) @@ -238,7 +246,7 @@ func TestListExperiments(t *testing.T) { // Access cache if data.err == "" { - response, err := svc.ListExperiments("litmus", "1") + response, err := svc.ListExperiments(expManagerName, "1") assert.Equal(t, data.expected, response) assert.NoError(t, err) } @@ -283,8 +291,8 @@ func TestListVariables(t *testing.T) { // Set up mock Experiment Managers expMgrSuccess := &mocks.StandardExperimentManager{} - expMgrSuccess.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrSuccess.On("IsCacheEnabled").Return(true) + expMgrSuccess.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrSuccess.On("IsCacheEnabled").Return(true, nil) expMgrSuccess.On("ListClients").Return(clients, nil) expMgrSuccess.On("ListVariablesForClient", client).Return(clientVariables, nil) expMgrSuccess.On("ListExperimentsForClient", client).Return(experiments, nil) @@ -293,34 +301,35 @@ func TestListVariables(t *testing.T) { Return(map[string][]manager.Variable{}, nil) expMgrFailure1 := &mocks.StandardExperimentManager{} - expMgrFailure1.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrFailure1.On("IsCacheEnabled").Return(true) + expMgrFailure1.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrFailure1.On("IsCacheEnabled").Return(true, nil) expMgrFailure1.On("ListClients").Return([]manager.Client{}, errors.New("List clients error")) expMgrFailure2 := &mocks.StandardExperimentManager{} - expMgrFailure2.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrFailure2.On("IsCacheEnabled").Return(true) + expMgrFailure2.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrFailure2.On("IsCacheEnabled").Return(true, nil) expMgrFailure2.On("ListClients").Return(clients, nil) expMgrFailure2.On("ListVariablesForClient", client). Return([]manager.Variable{}, errors.New("List client vars error")) expMgrFailure3 := &mocks.StandardExperimentManager{} - expMgrFailure3.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrFailure3.On("IsCacheEnabled").Return(true) + expMgrFailure3.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrFailure3.On("IsCacheEnabled").Return(true, nil) expMgrFailure3.On("ListClients").Return(clients, nil) expMgrFailure3.On("ListVariablesForClient", client).Return(clientVariables, nil) expMgrFailure3.On("ListExperimentsForClient", client). Return([]manager.Experiment{}, errors.New("List experiments error")) expMgrFailure4 := &mocks.StandardExperimentManager{} - expMgrFailure4.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig) - expMgrFailure4.On("IsCacheEnabled").Return(true) + expMgrFailure4.On("GetEngineInfo", mock.Anything).Return(standardExperimentManagerConfig, nil) + expMgrFailure4.On("IsCacheEnabled").Return(true, nil) expMgrFailure4.On("ListClients").Return(clients, nil) expMgrFailure4.On("ListVariablesForClient", client).Return(clientVariables, nil) expMgrFailure4.On("ListExperimentsForClient", client).Return(experiments, nil) expMgrFailure4.On("ListVariablesForExperiments", experiments). Return(map[string][]manager.Variable{}, errors.New("List experiment vars error")) + expManagerName := "exp-engine-002" // Define tests tests := map[string]struct { expMgr manager.ExperimentManager @@ -377,14 +386,14 @@ func TestListVariables(t *testing.T) { expMgr: expMgrSuccess, clientID: "1", experimentIDs: []string{"2"}, - badCacheKey: "engine:litmus:clients:1:variables", + badCacheKey: fmt.Sprintf("engine:%s:clients:1:variables", expManagerName), err: "Malformed variables info found in the cache for client 1", }, "failure | bad experiment vars cache": { expMgr: expMgrSuccess, clientID: "1", experimentIDs: []string{"2"}, - badCacheKey: "engine:litmus:experiments:2:variables", + badCacheKey: fmt.Sprintf("engine:%s:experiments:2:variables", expManagerName), err: "Malformed variables info found in the cache for experiment 2", }, } @@ -399,13 +408,13 @@ func TestListVariables(t *testing.T) { } svc := &experimentsService{ experimentManagers: map[string]manager.ExperimentManager{ - "litmus": data.expMgr, + expManagerName: data.expMgr, }, cache: cacheObj, } // Run and Validate - response, err := svc.ListVariables("litmus", data.clientID, data.experimentIDs) + response, err := svc.ListVariables(expManagerName, data.clientID, data.experimentIDs) // Sort items sort.SliceStable(response.Config, func(i, j int) bool { return response.Config[i].Name < response.Config[j].Name @@ -418,7 +427,7 @@ func TestListVariables(t *testing.T) { // Access cache if data.err == "" { - response, err := svc.ListVariables("litmus", data.clientID, data.experimentIDs) + response, err := svc.ListVariables(expManagerName, data.clientID, data.experimentIDs) // Sort items sort.SliceStable(response.Config, func(i, j int) bool { return response.Config[i].Name < response.Config[j].Name @@ -431,91 +440,102 @@ func TestListVariables(t *testing.T) { } func TestValidateExperimentConfig(t *testing.T) { - // Create mock experiment managers - stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{ - Name: "Litmus", - Type: manager.StandardExperimentManagerType, - StandardExperimentManagerConfig: &manager.StandardExperimentManagerConfig{ - ClientSelectionEnabled: true, - }, - }) - stdExpMgr.On("ValidateExperimentConfig", - &manager.StandardExperimentManagerConfig{ - ClientSelectionEnabled: true, - }, - manager.TuringExperimentConfig{}, - ).Return(nil) - - customExpMgr := &mocks.CustomExperimentManager{} - customExpMgr.On("GetEngineInfo").Return(customExperimentManagerConfig) - customExpMgr.On("ValidateExperimentConfig", []int{1, 2}).Return(nil) - - // Create test experiment service - expSvc := &experimentsService{ - experimentManagers: map[string]manager.ExperimentManager{ - "litmus": stdExpMgr, - "xp": customExpMgr, - }, - } - - // Define tests tests := map[string]struct { engine string - inputCfg interface{} - err string + inputCfg json.RawMessage + managers func(engine string, cfg json.RawMessage, err error) map[string]manager.ExperimentManager + err error }{ "failure | std exp mgr": { engine: "test-engine", - err: "Unknown experiment engine test-engine", + managers: func(engine string, cfg json.RawMessage, err error) map[string]manager.ExperimentManager { + return map[string]manager.ExperimentManager{} + }, + err: errors.New("Unknown experiment engine test-engine"), }, "success | std exp mgr": { - engine: "litmus", - inputCfg: manager.TuringExperimentConfig{}, + engine: "standard", + managers: func(engine string, cfg json.RawMessage, err error) map[string]manager.ExperimentManager { + stdExpMgr := &mocks.StandardExperimentManager{} + stdExpMgr. + On("ValidateExperimentConfig", cfg). + Return(err) + + return map[string]manager.ExperimentManager{ + engine: stdExpMgr, + } + }, + inputCfg: json.RawMessage(`{ + "client": {}, + "experiments": [], + "variables": {} + }`), }, "success | custom exp mgr": { - engine: "xp", - inputCfg: []int{1, 2}, + engine: "custom", + managers: func(engine string, cfg json.RawMessage, err error) map[string]manager.ExperimentManager { + customExpMgr := &mocks.ExperimentManager{} + customExpMgr.On("ValidateExperimentConfig", cfg).Return(err) + + return map[string]manager.ExperimentManager{ + engine: customExpMgr, + } + }, + inputCfg: json.RawMessage("[1, 2]"), + }, + "failure | validation errors": { + managers: func(engine string, cfg json.RawMessage, err error) map[string]manager.ExperimentManager { + customExpMgr := &mocks.ExperimentManager{} + customExpMgr. + On("ValidateExperimentConfig", cfg). + Return(err) + + return map[string]manager.ExperimentManager{ + engine: customExpMgr, + } + }, + inputCfg: json.RawMessage("[3, 4]"), + err: errors.New("validation error"), }, } // Run tests for name, data := range tests { t.Run(name, func(t *testing.T) { + expSvc := &experimentsService{ + experimentManagers: data.managers(data.engine, data.inputCfg, data.err), + } + err := expSvc.ValidateExperimentConfig(data.engine, data.inputCfg) - if data.err == "" { + if data.err == nil { assert.NoError(t, err) } else { - assert.Error(t, err) - if err != nil { - assert.Equal(t, data.err, err.Error()) - } + require.Error(t, err) + assert.EqualError(t, err, data.err.Error()) } }) } } func TestGetExperimentRunnerConfig(t *testing.T) { - expectedResult1 := json.RawMessage([]byte(`{"key": "value1"}`)) - expectedResult2 := json.RawMessage([]byte(`{"key": "value2"}`)) + expectedResult1 := json.RawMessage(`{"key": "value1"}`) + expectedResult2 := json.RawMessage(`{"key": "value2"}`) // Create mock experiment managers stdExpMgr := &mocks.StandardExperimentManager{} stdExpMgr.On("GetEngineInfo").Return(standardExperimentManagerConfig) - stdExpMgr.On("GetExperimentRunnerConfig", manager.TuringExperimentConfig{ - Client: manager.Client{ - ID: "1", - }, - }).Return(expectedResult1, nil) + stdExpMgr. + On("GetExperimentRunnerConfig", json.RawMessage(`{"client": {"id": "1"}}`)). + Return(expectedResult1, nil) - customExpMgr := &mocks.CustomExperimentManager{} + customExpMgr := &mocks.ExperimentManager{} customExpMgr.On("GetEngineInfo").Return(customExperimentManagerConfig) - customExpMgr.On("GetExperimentRunnerConfig", json.RawMessage([]byte(`[1,2]`))).Return(expectedResult2, nil) + customExpMgr.On("GetExperimentRunnerConfig", json.RawMessage(`[1,2]`)).Return(expectedResult2, nil) // Create test experiment service expSvc := &experimentsService{ experimentManagers: map[string]manager.ExperimentManager{ - "litmus": stdExpMgr, - "xp": customExpMgr, + "standard": stdExpMgr, + "custom": customExpMgr, }, } @@ -532,12 +552,12 @@ func TestGetExperimentRunnerConfig(t *testing.T) { err: "Unknown experiment engine test-engine", }, "success | std exp mgr": { - engine: "litmus", - inputCfg: json.RawMessage([]byte(`{"client": {"id": "1"}}`)), + engine: "standard", + inputCfg: json.RawMessage(`{"client": {"id": "1"}}`), expectedResult: expectedResult1, }, "success | custom exp mgr": { - engine: "xp", + engine: "custom", inputCfg: json.RawMessage(`[1,2]`), expectedResult: expectedResult2, }, diff --git a/api/turing/service/mocks/experiments_service.go b/api/turing/service/mocks/experiments_service.go index 57db45c16..6449ef351 100644 --- a/api/turing/service/mocks/experiments_service.go +++ b/api/turing/service/mocks/experiments_service.go @@ -3,10 +3,10 @@ package mocks import ( - "encoding/json" + json "encoding/json" - "github.com/gojek/turing/engines/experiment/manager" - "github.com/stretchr/testify/mock" + manager "github.com/gojek/turing/engines/experiment/manager" + mock "github.com/stretchr/testify/mock" ) // ExperimentsService is an autogenerated mock type for the ExperimentsService type @@ -15,11 +15,11 @@ type ExperimentsService struct { } // GetExperimentRunnerConfig provides a mock function with given fields: engine, cfg -func (_m *ExperimentsService) GetExperimentRunnerConfig(engine string, cfg interface{}) (json.RawMessage, error) { +func (_m *ExperimentsService) GetExperimentRunnerConfig(engine string, cfg json.RawMessage) (json.RawMessage, error) { ret := _m.Called(engine, cfg) var r0 json.RawMessage - if rf, ok := ret.Get(0).(func(string, interface{}) json.RawMessage); ok { + if rf, ok := ret.Get(0).(func(string, json.RawMessage) json.RawMessage); ok { r0 = rf(engine, cfg) } else { if ret.Get(0) != nil { @@ -28,7 +28,7 @@ func (_m *ExperimentsService) GetExperimentRunnerConfig(engine string, cfg inter } var r1 error - if rf, ok := ret.Get(1).(func(string, interface{}) error); ok { + if rf, ok := ret.Get(1).(func(string, json.RawMessage) error); ok { r1 = rf(engine, cfg) } else { r1 = ret.Error(1) @@ -37,27 +37,6 @@ func (_m *ExperimentsService) GetExperimentRunnerConfig(engine string, cfg inter return r0, r1 } -// GetStandardExperimentConfig provides a mock function with given fields: config -func (_m *ExperimentsService) GetStandardExperimentConfig(config interface{}) (manager.TuringExperimentConfig, error) { - ret := _m.Called(config) - - var r0 manager.TuringExperimentConfig - if rf, ok := ret.Get(0).(func(interface{}) manager.TuringExperimentConfig); ok { - r0 = rf(config) - } else { - r0 = ret.Get(0).(manager.TuringExperimentConfig) - } - - var r1 error - if rf, ok := ret.Get(1).(func(interface{}) error); ok { - r1 = rf(config) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // IsStandardExperimentManager provides a mock function with given fields: engine func (_m *ExperimentsService) IsStandardExperimentManager(engine string) bool { ret := _m.Called(engine) @@ -156,11 +135,11 @@ func (_m *ExperimentsService) ListVariables(engine string, clientID string, expe } // ValidateExperimentConfig provides a mock function with given fields: engine, cfg -func (_m *ExperimentsService) ValidateExperimentConfig(engine string, cfg interface{}) error { +func (_m *ExperimentsService) ValidateExperimentConfig(engine string, cfg json.RawMessage) error { ret := _m.Called(engine, cfg) var r0 error - if rf, ok := ret.Get(0).(func(string, interface{}) error); ok { + if rf, ok := ret.Get(0).(func(string, json.RawMessage) error); ok { r0 = rf(engine, cfg) } else { r0 = ret.Error(0) diff --git a/api/turing/validation/validator.go b/api/turing/validation/validator.go index 3cf61bbbc..157c39ecf 100644 --- a/api/turing/validation/validator.go +++ b/api/turing/validation/validator.go @@ -94,18 +94,25 @@ func validateLogConfig(sl validator.StructLevel) { } func newExperimentConfigValidator(expSvc service.ExperimentsService) func(validator.StructLevel) { + supportedEngines := make(map[string]bool) + supportedEnginesStr := models.ExperimentEngineTypeNop + for _, engine := range expSvc.ListEngines() { + supportedEnginesStr = fmt.Sprintf("%s,%s", supportedEnginesStr, engine.Name) + supportedEngines[engine.Name] = true + } + validationFunc := func(sl validator.StructLevel) { field := sl.Current().Interface().(request.ExperimentEngineConfig) - switch field.Type { - case string(models.ExperimentEngineTypeNop): + switch { + case field.Type == models.ExperimentEngineTypeNop: return - case string(models.ExperimentEngineTypeLitmus), string(models.ExperimentEngineTypeXp): + case supportedEngines[field.Type]: err := expSvc.ValidateExperimentConfig(field.Type, field.Config) if err != nil { sl.ReportError(field.Config, "config", "ExperimentEngineConfig.Config", err.Error(), "") } default: - sl.ReportError(field.Type, "type", "Type", "oneof", "litmus,xp,nop") + sl.ReportError(field.Type, "type", "Type", "oneof", supportedEnginesStr) } } return validationFunc diff --git a/api/turing/validation/validator_test.go b/api/turing/validation/validator_test.go index c6d833be6..7f7546bed 100644 --- a/api/turing/validation/validator_test.go +++ b/api/turing/validation/validator_test.go @@ -3,45 +3,40 @@ package validation_test import ( + "encoding/json" "errors" - "strings" + "fmt" "testing" - "github.com/gojek/turing/engines/router" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/gojek/turing/api/turing/api/request" "github.com/gojek/turing/api/turing/models" "github.com/gojek/turing/api/turing/service/mocks" "github.com/gojek/turing/api/turing/validation" "github.com/gojek/turing/engines/experiment/manager" - request2 "github.com/gojek/turing/engines/experiment/pkg/request" + expRequest "github.com/gojek/turing/engines/experiment/pkg/request" + "github.com/gojek/turing/engines/router" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestValidateLogConfig(t *testing.T) { - tt := []struct { - name string + tt := map[string]struct { input request.LogConfig hasErr bool }{ - { - name: "valid_nop", + "valid_nop": { input: request.LogConfig{ ResultLoggerType: "nop", }, hasErr: false, }, - { - name: "invalid_type", + "invalid_type": { input: request.LogConfig{ ResultLoggerType: "nope", }, hasErr: true, }, - { - name: "valid_bq", + "valid_bq": { input: request.LogConfig{ ResultLoggerType: "bigquery", BigQueryConfig: &request.BigQueryConfig{ @@ -51,15 +46,13 @@ func TestValidateLogConfig(t *testing.T) { }, hasErr: false, }, - { - name: "bq_missing_config", + "bq_missing_config": { input: request.LogConfig{ ResultLoggerType: "bigquery", }, hasErr: true, }, - { - name: "bq_invalid_table", + "bq_invalid_table": { input: request.LogConfig{ ResultLoggerType: "bigquery", BigQueryConfig: &request.BigQueryConfig{ @@ -69,8 +62,7 @@ func TestValidateLogConfig(t *testing.T) { }, hasErr: true, }, - { - name: "bq_invalid_svc_account", + "bq_invalid_svc_account": { input: request.LogConfig{ ResultLoggerType: "bigquery", BigQueryConfig: &request.BigQueryConfig{ @@ -80,8 +72,7 @@ func TestValidateLogConfig(t *testing.T) { }, hasErr: true, }, - { - name: "kafka_valid_config", + "kafka_valid_config": { input: request.LogConfig{ ResultLoggerType: "kafka", KafkaConfig: &request.KafkaConfig{ @@ -92,15 +83,13 @@ func TestValidateLogConfig(t *testing.T) { }, hasErr: false, }, - { - name: "kafka_missing_config", + "kafka_missing_config": { input: request.LogConfig{ ResultLoggerType: "kafka", }, hasErr: true, }, - { - name: "kafka_invalid_config_missing_brokers", + "kafka_invalid_config_missing_brokers": { input: request.LogConfig{ ResultLoggerType: "kafka", KafkaConfig: &request.KafkaConfig{ @@ -110,8 +99,7 @@ func TestValidateLogConfig(t *testing.T) { }, hasErr: true, }, - { - name: "kafka_invalid_config_missing_topic", + "kafka_invalid_config_missing_topic": { input: request.LogConfig{ ResultLoggerType: "kafka", KafkaConfig: &request.KafkaConfig{ @@ -121,8 +109,7 @@ func TestValidateLogConfig(t *testing.T) { }, hasErr: true, }, - { - name: "kafka_invalid_config_invalid_serialization", + "kafka_invalid_config_invalid_serialization": { input: request.LogConfig{ ResultLoggerType: "kafka", KafkaConfig: &request.KafkaConfig{ @@ -135,9 +122,11 @@ func TestValidateLogConfig(t *testing.T) { }, } - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - validate, err := validation.NewValidator(&mocks.ExperimentsService{}) + for name, tc := range tt { + t.Run(name, func(t *testing.T) { + mockExperimentsService := &mocks.ExperimentsService{} + mockExperimentsService.On("ListEngines").Return([]manager.Engine{}) + validate, err := validation.NewValidator(mockExperimentsService) assert.NoError(t, err) err = validate.Struct(&tc.input) assert.Equal(t, tc.hasErr, err != nil) @@ -146,56 +135,62 @@ func TestValidateLogConfig(t *testing.T) { } func TestValidateExperimentEngineConfig(t *testing.T) { + validationErr := "test-error" // Create mock experiment service - client1 := manager.Client{ID: "1", Username: "1"} - client2 := manager.Client{ID: "2", Username: "2"} + config1 := json.RawMessage(`{"client": {"id": "1", "username": "1"}}`) + config2 := json.RawMessage(`{"client": {"id": "2", "username": "2"}}`) expSvc := &mocks.ExperimentsService{} - expSvc.On("ValidateExperimentConfig", "xp", manager.TuringExperimentConfig{Client: client1}). + expSvc.On("ListEngines").Return([]manager.Engine{{Name: "custom"}}) + expSvc.On("ValidateExperimentConfig", "custom", config1). Return(nil) - expSvc.On("ValidateExperimentConfig", "xp", manager.TuringExperimentConfig{Client: client2}). - Return(errors.New("test-error")) + expSvc.On("ValidateExperimentConfig", "custom", config2). + Return(errors.New(validationErr)) // Define tests - tests := []struct { - name string - input request.ExperimentEngineConfig - hasErr bool + tests := map[string]struct { + input request.ExperimentEngineConfig + err string }{ - { - name: "valid_nop", + "success | valid nop": { input: request.ExperimentEngineConfig{ Type: "nop", }, - hasErr: false, }, - { - name: "valid_exp_config", + "success | valid experiment config": { input: request.ExperimentEngineConfig{ - Type: "xp", - Config: manager.TuringExperimentConfig{ - Client: client1, - }, + Type: "custom", + Config: config1, }, - hasErr: false, }, - { - name: "invalid_exp_config", + "failure | unknown engine type": { input: request.ExperimentEngineConfig{ - Type: "unknown", - Config: manager.TuringExperimentConfig{ - Client: client2, - }, + Type: "unknown", + Config: config2, }, - hasErr: true, + err: "Key: 'ExperimentEngineConfig.type' " + + "Error:Field validation for 'type' failed on the 'oneof' tag", + }, + "failure | validation error": { + input: request.ExperimentEngineConfig{ + Type: "custom", + Config: config2, + }, + err: fmt.Sprintf( + "Key: 'ExperimentEngineConfig.config' "+ + "Error:Field validation for 'config' failed on the '%s' tag", validationErr), }, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { + for name, tc := range tests { + t.Run(name, func(t *testing.T) { validate, err := validation.NewValidator(expSvc) assert.NoError(t, err) err = validate.Struct(&tc.input) - assert.Equal(t, tc.hasErr, err != nil) + if tc.err == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tc.err) + } }) } } @@ -244,7 +239,7 @@ func TestValidateTrafficRules(t *testing.T) { { Conditions: []*router.TrafficRuleCondition{ { - FieldSource: request2.HeaderFieldSource, + FieldSource: expRequest.HeaderFieldSource, Field: "X-Region", Operator: router.InConditionOperator, Values: []string{"region-a", "region-b"}, @@ -293,7 +288,7 @@ func TestValidateTrafficRules(t *testing.T) { { Conditions: []*router.TrafficRuleCondition{ { - FieldSource: request2.HeaderFieldSource, + FieldSource: expRequest.HeaderFieldSource, Field: "X-Region", Operator: router.InConditionOperator, Values: []string{"region-b"}, @@ -325,7 +320,7 @@ func TestValidateTrafficRules(t *testing.T) { { Conditions: []*router.TrafficRuleCondition{ { - FieldSource: request2.HeaderFieldSource, + FieldSource: expRequest.HeaderFieldSource, Field: "X-Region", Operator: router.RuleConditionOperator{}, Values: []string{"region-b"}, @@ -389,7 +384,7 @@ func TestValidateTrafficRules(t *testing.T) { { Conditions: []*router.TrafficRuleCondition{ { - FieldSource: request2.HeaderFieldSource, + FieldSource: expRequest.HeaderFieldSource, Field: "", Operator: router.InConditionOperator, Values: []string{}, @@ -417,7 +412,7 @@ func TestValidateTrafficRules(t *testing.T) { { Conditions: []*router.TrafficRuleCondition{ { - FieldSource: request2.PayloadFieldSource, + FieldSource: expRequest.PayloadFieldSource, Field: "some_property", Operator: router.InConditionOperator, Values: []string{"some_value"}, @@ -443,7 +438,7 @@ func TestValidateTrafficRules(t *testing.T) { { Conditions: []*router.TrafficRuleCondition{ { - FieldSource: request2.PayloadFieldSource, + FieldSource: expRequest.PayloadFieldSource, Field: "some_property", Operator: router.InConditionOperator, Values: []string{"some_value"}, @@ -459,7 +454,9 @@ func TestValidateTrafficRules(t *testing.T) { for name, tt := range suite { t.Run(name, func(t *testing.T) { - validate, err := validation.NewValidator(&mocks.ExperimentsService{}) + mockExperimentsService := &mocks.ExperimentsService{} + mockExperimentsService.On("ListEngines").Return([]manager.Engine{}) + validate, err := validation.NewValidator(mockExperimentsService) require.NoError(t, err) err = validate.Struct(tt.RouterConfig()) @@ -471,16 +468,3 @@ func TestValidateTrafficRules(t *testing.T) { }) } } - -type DNSTestStruct struct { - Name string `validate:"dns,lte=50,gte=3"` -} - -func genString(length int) string { - var sb strings.Builder - for i := 0; i < length; i++ { - sb.WriteString("a") - } - - return sb.String() -} diff --git a/engines/experiment/examples/plugins/nop/manager.go b/engines/experiment/examples/plugins/nop/manager.go index 014a8eb05..fe53644e5 100644 --- a/engines/experiment/examples/plugins/nop/manager.go +++ b/engines/experiment/examples/plugins/nop/manager.go @@ -21,7 +21,7 @@ func (m *ExperimentManager) Configure(rawCfg json.RawMessage) error { return nil } -func (m *ExperimentManager) GetEngineInfo() manager.Engine { +func (m *ExperimentManager) GetEngineInfo() (manager.Engine, error) { return manager.Engine{ Name: "nop", DisplayName: m.displayName, @@ -31,5 +31,13 @@ func (m *ExperimentManager) GetEngineInfo() manager.Engine { ExperimentSelectionEnabled: false, HomePageURL: "http://example.com", }, - } + }, nil +} + +func (*ExperimentManager) ValidateExperimentConfig(json.RawMessage) error { + return nil +} + +func (*ExperimentManager) GetExperimentRunnerConfig(json.RawMessage) (json.RawMessage, error) { + return json.RawMessage{}, nil } diff --git a/engines/experiment/manager/adapter.go b/engines/experiment/manager/adapter.go index 43560553a..cf0ffcbae 100644 --- a/engines/experiment/manager/adapter.go +++ b/engines/experiment/manager/adapter.go @@ -5,31 +5,35 @@ package manager */ import ( - "encoding/json" "errors" - "fmt" ) const ( - experimentManagerCastingErr = "Error casting %s to %s experiment manager" - standardExperimentConfigErr = "Unable to parse standard experiment config: %v" - standardMethodErr = "Method is only supported by standard experiment managers" - unknownManagerTypeErr = "Experiment Manager type %s is not recognized" + standardMethodErr = "Method is only supported by standard experiment managers" ) +func IsStandardExperimentManager(expManager ExperimentManager) bool { + engineInfo, err := expManager.GetEngineInfo() + return err == nil && engineInfo.Type == StandardExperimentManagerType +} + // StandardExperimentManager methods ****************************************** func IsCacheEnabled(expManager ExperimentManager) bool { - if expManager.GetEngineInfo().Type == StandardExperimentManagerType { + if IsStandardExperimentManager(expManager) { if stdMgr, ok := expManager.(StandardExperimentManager); ok { - return stdMgr.IsCacheEnabled() + cacheEnabled, err := stdMgr.IsCacheEnabled() + if err != nil { + return false + } + return cacheEnabled } } return false } func ListClients(expManager ExperimentManager) ([]Client, error) { - if expManager.GetEngineInfo().Type == StandardExperimentManagerType { + if IsStandardExperimentManager(expManager) { if stdMgr, ok := expManager.(StandardExperimentManager); ok { return stdMgr.ListClients() } @@ -38,7 +42,7 @@ func ListClients(expManager ExperimentManager) ([]Client, error) { } func ListExperiments(expManager ExperimentManager) ([]Experiment, error) { - if expManager.GetEngineInfo().Type == StandardExperimentManagerType { + if IsStandardExperimentManager(expManager) { if stdMgr, ok := expManager.(StandardExperimentManager); ok { return stdMgr.ListExperiments() } @@ -47,7 +51,7 @@ func ListExperiments(expManager ExperimentManager) ([]Experiment, error) { } func ListExperimentsForClient(expManager ExperimentManager, client Client) ([]Experiment, error) { - if expManager.GetEngineInfo().Type == StandardExperimentManagerType { + if IsStandardExperimentManager(expManager) { if stdMgr, ok := expManager.(StandardExperimentManager); ok { return stdMgr.ListExperimentsForClient(client) } @@ -56,7 +60,7 @@ func ListExperimentsForClient(expManager ExperimentManager, client Client) ([]Ex } func ListVariablesForClient(expManager ExperimentManager, client Client) ([]Variable, error) { - if expManager.GetEngineInfo().Type == StandardExperimentManagerType { + if IsStandardExperimentManager(expManager) { if stdMgr, ok := expManager.(StandardExperimentManager); ok { return stdMgr.ListVariablesForClient(client) } @@ -65,62 +69,10 @@ func ListVariablesForClient(expManager ExperimentManager, client Client) ([]Vari } func ListVariablesForExperiments(expManager ExperimentManager, exps []Experiment) (map[string][]Variable, error) { - if expManager.GetEngineInfo().Type == StandardExperimentManagerType { + if IsStandardExperimentManager(expManager) { if stdMgr, ok := expManager.(StandardExperimentManager); ok { return stdMgr.ListVariablesForExperiments(exps) } } return map[string][]Variable{}, errors.New(standardMethodErr) } - -// Common methods ************************************************************* - -func GetExperimentRunnerConfig(expManager ExperimentManager, expCfg interface{}) (json.RawMessage, error) { - engineInfo := expManager.GetEngineInfo() - // Call the appropriate validator method based on the type of the experiment manager - switch engineInfo.Type { - case StandardExperimentManagerType: - stdMgr, ok := expManager.(StandardExperimentManager) - if !ok { - return json.RawMessage{}, fmt.Errorf(experimentManagerCastingErr, engineInfo.Name, "standard") - } - stdExpConfig, err := GetStandardExperimentConfig(expCfg) - if err != nil { - return json.RawMessage{}, fmt.Errorf(standardExperimentConfigErr, err) - } - return stdMgr.GetExperimentRunnerConfig(stdExpConfig) - case CustomExperimentManagerType: - customMgr, ok := expManager.(CustomExperimentManager) - if !ok { - return json.RawMessage{}, fmt.Errorf(experimentManagerCastingErr, engineInfo.Name, "custom") - } - return customMgr.GetExperimentRunnerConfig(expCfg) - default: - return nil, fmt.Errorf(unknownManagerTypeErr, engineInfo.Type) - } -} - -func ValidateExperimentConfig(expManager ExperimentManager, expCfg interface{}) error { - engineInfo := expManager.GetEngineInfo() - // Call the appropriate validator method based on the type of the experiment manager - switch engineInfo.Type { - case StandardExperimentManagerType: - stdMgr, ok := expManager.(StandardExperimentManager) - if !ok { - return fmt.Errorf(experimentManagerCastingErr, engineInfo.Name, "standard") - } - stdExpConfig, err := GetStandardExperimentConfig(expCfg) - if err != nil { - return fmt.Errorf(standardExperimentConfigErr, err) - } - return stdMgr.ValidateExperimentConfig(engineInfo.StandardExperimentManagerConfig, stdExpConfig) - case CustomExperimentManagerType: - customMgr, ok := expManager.(CustomExperimentManager) - if !ok { - return fmt.Errorf(experimentManagerCastingErr, engineInfo.Name, "custom") - } - return customMgr.ValidateExperimentConfig(expCfg) - default: - return fmt.Errorf(unknownManagerTypeErr, engineInfo.Type) - } -} diff --git a/engines/experiment/manager/adapter_test.go b/engines/experiment/manager/adapter_test.go index 8b50eeff4..c52f3f9ac 100644 --- a/engines/experiment/manager/adapter_test.go +++ b/engines/experiment/manager/adapter_test.go @@ -1,33 +1,25 @@ package manager_test import ( - "encoding/json" - "errors" - "io/ioutil" - "path/filepath" - "strings" "testing" "github.com/gojek/turing/engines/experiment/manager" "github.com/gojek/turing/engines/experiment/manager/mocks" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" ) func TestStandardExperimentIsCacheEnabled(t *testing.T) { // Set up mocks stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) - stdExpMgr.On("IsCacheEnabled").Return(true) + stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}, nil) + stdExpMgr.On("IsCacheEnabled").Return(true, nil) expMgr := &mocks.ExperimentManager{} - expMgr.On("GetEngineInfo").Return(manager.Engine{}) + expMgr.On("GetEngineInfo").Return(manager.Engine{}, nil) - // Validate // IsCacheEnabled - assert.Equal(t, true, manager.IsCacheEnabled(stdExpMgr)) - assert.Equal(t, false, manager.IsCacheEnabled(expMgr)) + assert.True(t, manager.IsCacheEnabled(stdExpMgr)) + assert.False(t, manager.IsCacheEnabled(expMgr)) } func TestStandardExperimentListClients(t *testing.T) { @@ -35,11 +27,11 @@ func TestStandardExperimentListClients(t *testing.T) { // Set up mocks stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) + stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}, nil) stdExpMgr.On("ListClients").Return([]manager.Client{{}}, nil) expMgr := &mocks.ExperimentManager{} - expMgr.On("GetEngineInfo").Return(manager.Engine{}) + expMgr.On("GetEngineInfo").Return(manager.Engine{}, nil) // Validate // ListClients @@ -56,11 +48,11 @@ func TestStandardExperimentListExperiments(t *testing.T) { // Set up mocks stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) + stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}, nil) stdExpMgr.On("ListExperiments").Return([]manager.Experiment{{}}, nil) expMgr := &mocks.ExperimentManager{} - expMgr.On("GetEngineInfo").Return(manager.Engine{}) + expMgr.On("GetEngineInfo").Return(manager.Engine{}, nil) // Validate // ListExperiments @@ -77,12 +69,12 @@ func TestStandardExperimentListExperimentsForClient(t *testing.T) { // Set up mocks stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) + stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}, nil) stdExpMgr.On("ListExperimentsForClient", manager.Client{ID: "1"}). Return([]manager.Experiment{{}}, nil) expMgr := &mocks.ExperimentManager{} - expMgr.On("GetEngineInfo").Return(manager.Engine{}) + expMgr.On("GetEngineInfo").Return(manager.Engine{}, nil) // Validate // ListExperimentsForClient @@ -99,12 +91,12 @@ func TestStandardExperimentListVariablesForClient(t *testing.T) { // Set up mocks stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) + stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}, nil) stdExpMgr.On("ListVariablesForClient", manager.Client{ID: "1"}). Return([]manager.Variable{{Name: "var-1"}}, nil) expMgr := &mocks.ExperimentManager{} - expMgr.On("GetEngineInfo").Return(manager.Engine{}) + expMgr.On("GetEngineInfo").Return(manager.Engine{}, nil) // Validate // ListVariablesForClient @@ -121,14 +113,14 @@ func TestStandardExperimentListVariablesForExperiments(t *testing.T) { // Set up mocks stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) + stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}, nil) stdExpMgr.On("ListVariablesForExperiments", []manager.Experiment{{}}). Return(map[string][]manager.Variable{ "test-exp": {{Name: "var-1"}}, }, nil) expMgr := &mocks.ExperimentManager{} - expMgr.On("GetEngineInfo").Return(manager.Engine{}) + expMgr.On("GetEngineInfo").Return(manager.Engine{}, nil) // Validate // ListVariablesForExperiments @@ -141,176 +133,3 @@ func TestStandardExperimentListVariablesForExperiments(t *testing.T) { assert.Equal(t, map[string][]manager.Variable{}, variablesMap) assert.EqualError(t, err, stdExpMgrErr) } - -func TestGetExperimentRunnerConfig(t *testing.T) { - testStdExpConfig := manager.TuringExperimentConfig{ - Client: manager.Client{Username: "client_name"}, - Experiments: []manager.Experiment{{Name: "exp_name"}}, - Variables: manager.Variables{ClientVariables: []manager.Variable{{Name: "var_name"}}}, - } - - // Get test data - testData, err := ioutil.ReadFile(filepath.Join("testdata", "experiment_runner_config.json")) - require.NoError(t, err) - var testRawConfig interface{} - err = json.Unmarshal(testData, &testRawConfig) - assert.NoError(t, err) - testSuccessResponse := json.RawMessage([]byte(`{}`)) - - // Set up mock experiment managers - // Standard Exp Manager - stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) - stdExpMgr.On("GetExperimentRunnerConfig", testStdExpConfig).Return(testSuccessResponse, nil) - stdExpMgr.On("GetExperimentRunnerConfig", mock.Anything).Return(nil, errors.New("Unexpected Parameters")) - // Custom Exp Manager - customExpMgr := &mocks.CustomExperimentManager{} - customExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.CustomExperimentManagerType}) - customExpMgr.On("GetExperimentRunnerConfig", testRawConfig).Return(testSuccessResponse, nil) - customExpMgr.On("GetExperimentRunnerConfig", mock.Anything).Return(nil, errors.New("Unexpected Parameters")) - // Bad Experiment Managers - badStdExpMgr := &mocks.ExperimentManager{} - badStdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) - badCustomExpMgr := &mocks.ExperimentManager{} - badCustomExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.CustomExperimentManagerType}) - badExpMgr := &mocks.ExperimentManager{} - badExpMgr.On("GetEngineInfo").Return(manager.Engine{}) - - // Set up tests - tests := map[string]struct { - mgr manager.ExperimentManager - rawCfg interface{} - expected json.RawMessage - err string - }{ - "success | standard": { - mgr: stdExpMgr, - rawCfg: testStdExpConfig, - expected: testSuccessResponse, - }, - "success | custom": { - mgr: customExpMgr, - rawCfg: testRawConfig, - expected: testSuccessResponse, - }, - "failure | std mismatched type": { - mgr: badStdExpMgr, - err: "Error casting to standard experiment manager", - }, - "failure | std config error": { - mgr: stdExpMgr, - rawCfg: []int{}, - err: strings.Join([]string{ - "Unable to parse standard experiment config: ", - "json: cannot unmarshal array into Go value of type manager.TuringExperimentConfig"}, ""), - }, - "failure | custom mismatched type": { - mgr: badCustomExpMgr, - err: "Error casting to custom experiment manager", - }, - "failure | unknown exp manager type": { - mgr: badExpMgr, - err: "Experiment Manager type is not recognized", - }, - } - - // Test calls to the correct experiment manager method, based on the type - for name, data := range tests { - t.Run(name, func(t *testing.T) { - resp, err := manager.GetExperimentRunnerConfig(data.mgr, data.rawCfg) - if data.err != "" { - assert.EqualError(t, err, data.err) - } else { - assert.NoError(t, err) - assert.Equal(t, data.expected, resp) - } - }) - } -} - -func TestAdapterValidateExperimentConfig(t *testing.T) { - testStdExpConfig := manager.TuringExperimentConfig{ - Client: manager.Client{Username: "client_name"}, - Experiments: []manager.Experiment{{Name: "exp_name"}}, - Variables: manager.Variables{ClientVariables: []manager.Variable{{Name: "var_name"}}}, - } - - var testRawConfig interface{} - err := json.Unmarshal([]byte(`{ - "client": {"username": "client_name"}, - "experiments": [{"name": "exp_name"}], - "variables": {"client_variables": [{"name": "var_name"}]} - }`), &testRawConfig) - assert.NoError(t, err) - testStdExpMgrConfig := &manager.StandardExperimentManagerConfig{} - - // Set up mock experiment managers - // Standard Exp Manager - stdExpMgr := &mocks.StandardExperimentManager{} - stdExpMgr.On("GetEngineInfo").Return(manager.Engine{ - Type: manager.StandardExperimentManagerType, - StandardExperimentManagerConfig: testStdExpMgrConfig, - }) - stdExpMgr.On("ValidateExperimentConfig", testStdExpMgrConfig, testStdExpConfig).Return(nil) - stdExpMgr.On("ValidateExperimentConfig", mock.Anything).Return(errors.New("Unexpected Parameters")) - // Custom Exp Manager - customExpMgr := &mocks.CustomExperimentManager{} - customExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.CustomExperimentManagerType}) - customExpMgr.On("ValidateExperimentConfig", testRawConfig).Return(nil) - customExpMgr.On("ValidateExperimentConfig", mock.Anything).Return(errors.New("Unexpected Parameters")) - - // Bad Experiment Managers - badStdExpMgr := &mocks.ExperimentManager{} - badStdExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.StandardExperimentManagerType}) - badCustomExpMgr := &mocks.ExperimentManager{} - badCustomExpMgr.On("GetEngineInfo").Return(manager.Engine{Type: manager.CustomExperimentManagerType}) - badExpMgr := &mocks.ExperimentManager{} - badExpMgr.On("GetEngineInfo").Return(manager.Engine{}) - - // Set up tests - tests := map[string]struct { - mgr manager.ExperimentManager - rawCfg interface{} - err string - }{ - "success | standard": { - mgr: stdExpMgr, - rawCfg: testStdExpConfig, - }, - "success | custom": { - mgr: customExpMgr, - rawCfg: testRawConfig, - }, - "failure | std mismatched type": { - mgr: badStdExpMgr, - err: "Error casting to standard experiment manager", - }, - "failure | std config error": { - mgr: stdExpMgr, - rawCfg: []int{}, - err: strings.Join([]string{ - "Unable to parse standard experiment config: ", - "json: cannot unmarshal array into Go value of type manager.TuringExperimentConfig"}, ""), - }, - "failure | custom mismatched type": { - mgr: badCustomExpMgr, - err: "Error casting to custom experiment manager", - }, - "failure | unknown exp manager type": { - mgr: badExpMgr, - err: "Experiment Manager type is not recognized", - }, - } - - // Test calls to the correct experiment manager method, based on the type - for name, data := range tests { - t.Run(name, func(t *testing.T) { - err := manager.ValidateExperimentConfig(data.mgr, data.rawCfg) - if data.err != "" { - assert.EqualError(t, err, data.err) - } else { - assert.NoError(t, err) - } - }) - } -} diff --git a/engines/experiment/manager/base_standard_manager.go b/engines/experiment/manager/base_standard_manager.go index 8cd838e56..9430398ce 100644 --- a/engines/experiment/manager/base_standard_manager.go +++ b/engines/experiment/manager/base_standard_manager.go @@ -2,6 +2,7 @@ package manager import ( "context" + "encoding/json" "errors" "strings" @@ -14,10 +15,15 @@ import ( // implementations of the interface to provide the base behavior. type BaseStandardExperimentManager struct { validate *validator.Validate + info Engine } -func (*BaseStandardExperimentManager) IsCacheEnabled() bool { - return true +func (em *BaseStandardExperimentManager) GetEngineInfo() (Engine, error) { + return em.info, nil +} + +func (*BaseStandardExperimentManager) IsCacheEnabled() (bool, error) { + return true, nil } func (*BaseStandardExperimentManager) ListClients() ([]Client, error) { @@ -40,14 +46,17 @@ func (*BaseStandardExperimentManager) ListVariablesForExperiments([]Experiment) return make(map[string][]Variable), nil } -func (em *BaseStandardExperimentManager) ValidateExperimentConfig( - engineCfg *StandardExperimentManagerConfig, - experimentCfg TuringExperimentConfig, -) error { +func (em *BaseStandardExperimentManager) ValidateExperimentConfig(cfg json.RawMessage) error { + engineCfg := em.info.StandardExperimentManagerConfig if engineCfg == nil { return errors.New("Missing Standard Engine configuration") } + var experimentCfg TuringExperimentConfig + if err := json.Unmarshal(cfg, &experimentCfg); err != nil { + return err + } + if engineCfg.ExperimentSelectionEnabled { // Check that there is at least 1 experiment if len(experimentCfg.Experiments) < 1 { @@ -86,8 +95,9 @@ func (em *BaseStandardExperimentManager) ValidateExperimentConfig( } // NewBaseStandardExperimentManager is a constructor for the base experiment manager -func NewBaseStandardExperimentManager() *BaseStandardExperimentManager { +func NewBaseStandardExperimentManager(info Engine) *BaseStandardExperimentManager { return &BaseStandardExperimentManager{ + info: info, validate: newExperimentConfigValidator(), } } diff --git a/engines/experiment/manager/base_standard_manager_test.go b/engines/experiment/manager/base_standard_manager_test.go index 2a4c6658b..970ec4857 100644 --- a/engines/experiment/manager/base_standard_manager_test.go +++ b/engines/experiment/manager/base_standard_manager_test.go @@ -1,6 +1,7 @@ package manager_test import ( + "encoding/json" "strings" "testing" @@ -11,7 +12,9 @@ import ( func TestBaseStandardExperimentManagerMethods(t *testing.T) { em := &manager.BaseStandardExperimentManager{} - assert.Equal(t, true, em.IsCacheEnabled()) + actual, err := em.IsCacheEnabled() + assert.NoError(t, err) + assert.True(t, actual) // Get clients clients, err := em.ListClients() @@ -36,9 +39,6 @@ func TestBaseStandardExperimentManagerMethods(t *testing.T) { } func TestValidateExperimentConfig(t *testing.T) { - em := manager.NewBaseStandardExperimentManager() - - // Define tests tests := map[string]struct { engine manager.Engine cfg manager.TuringExperimentConfig @@ -167,7 +167,10 @@ func TestValidateExperimentConfig(t *testing.T) { // Run tests for name, data := range tests { t.Run(name, func(t *testing.T) { - err := em.ValidateExperimentConfig(data.engine.StandardExperimentManagerConfig, data.cfg) + em := manager.NewBaseStandardExperimentManager(data.engine) + + cfg, _ := json.Marshal(data.cfg) + err := em.ValidateExperimentConfig(cfg) if data.err != "" { // Expect error assert.Error(t, err) diff --git a/engines/experiment/manager/manager.go b/engines/experiment/manager/manager.go index 71251e86d..bd42da5f5 100644 --- a/engines/experiment/manager/manager.go +++ b/engines/experiment/manager/manager.go @@ -9,24 +9,26 @@ import ( // experiments on Turing. type ExperimentManager interface { // GetEngineInfo returns the configuration of the experiment engine - GetEngineInfo() Engine + GetEngineInfo() (Engine, error) + + // ValidateExperimentConfig validates the given Turing experiment config for the expected data and format + ValidateExperimentConfig(cfg json.RawMessage) error + + // GetExperimentRunnerConfig converts the given config (as retrieved from the DB) into a format suitable + // for the Turing router (i.e., to be passed to the Experiment Runner). This interface method will be + // called at the time of router deployment. + // + // cfg holds the experiment configuration in a format that is suitable for use with the Turing UI and + // this is the data that is saved to the Turing DB. + // + // In case of StandardExperimentManager, cfg is expected to be unmarshalled into TuringExperimentConfig + GetExperimentRunnerConfig(cfg json.RawMessage) (json.RawMessage, error) } type StandardExperimentManager interface { ExperimentManager - - // GetExperimentRunnerConfig converts the given TuringExperimentConfig into a format suitable for the - // Turing router. TuringExperimentConfig holds the experiment configuration in a format that is suitable - // for use with the Turing UI and this is the data that is saved to the Turing DB. This interface method - // will be called at the time of router deployment to convert the data into the format that the router, i.e., - // Experiment Runner expects. - GetExperimentRunnerConfig(TuringExperimentConfig) (json.RawMessage, error) - - // BaseStandardExperimentManager provides default implementations for the following methods - // that may be composed into the experiment engine. - // IsCacheEnabled returns whether the experiment engine wants to cache its responses in the Turing API cache - IsCacheEnabled() bool + IsCacheEnabled() (bool, error) // ListClients returns a list of the clients registered on the experiment engine ListClients() ([]Client, error) // ListExperiments returns a list of the experiments registered on the experiment engine @@ -38,32 +40,13 @@ type StandardExperimentManager interface { ListVariablesForClient(Client) ([]Variable, error) // ListVariablesForExperiments returns a list of the variables registered on the given experiments ListVariablesForExperiments([]Experiment) (map[string][]Variable, error) - // ValidateExperimentConfig validates the given Turing experiment config for the expected data and format, - // based on the given engine's properties. - ValidateExperimentConfig(*StandardExperimentManagerConfig, TuringExperimentConfig) error } type CustomExperimentManager interface { ExperimentManager - - // GetExperimentRunnerConfig converts the given config (as retrieved from the DB) into a format suitable - // for the Turing router (i.e., to be passed to the Experiment Runner). This interface method will be - // called at the time of router deployment. - GetExperimentRunnerConfig(interface{}) (json.RawMessage, error) - // ValidateExperimentConfig validates the given Turing experiment config for the expected data and format - ValidateExperimentConfig(interface{}) error } -func GetStandardExperimentConfig(cfg interface{}) (TuringExperimentConfig, error) { - var stdExpCfg TuringExperimentConfig - - // Marshal to json - bytes, err := json.Marshal(cfg) - if err != nil { - return stdExpCfg, err - } - - // Unmarshal using the TuringExperimentConfig type - err = json.Unmarshal(bytes, &stdExpCfg) - return stdExpCfg, err +func ParseStandardExperimentConfig(raw json.RawMessage) (stdExpCfg TuringExperimentConfig, err error) { + err = json.Unmarshal(raw, &stdExpCfg) + return } diff --git a/engines/experiment/manager/mocks/CustomExperimentManager.go b/engines/experiment/manager/mocks/CustomExperimentManager.go deleted file mode 100644 index 136e0e684..000000000 --- a/engines/experiment/manager/mocks/CustomExperimentManager.go +++ /dev/null @@ -1,66 +0,0 @@ -// Code generated by mockery v2.9.4. DO NOT EDIT. - -package mocks - -import ( - json "encoding/json" - - "github.com/gojek/turing/engines/experiment/manager" - mock "github.com/stretchr/testify/mock" -) - -// CustomExperimentManager is an autogenerated mock type for the CustomExperimentManager type -type CustomExperimentManager struct { - mock.Mock -} - -// GetEngineInfo provides a mock function with given fields: -func (_m *CustomExperimentManager) GetEngineInfo() manager.Engine { - ret := _m.Called() - - var r0 manager.Engine - if rf, ok := ret.Get(0).(func() manager.Engine); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(manager.Engine) - } - - return r0 -} - -// GetExperimentRunnerConfig provides a mock function with given fields: _a0 -func (_m *CustomExperimentManager) GetExperimentRunnerConfig(_a0 interface{}) (json.RawMessage, error) { - ret := _m.Called(_a0) - - var r0 json.RawMessage - if rf, ok := ret.Get(0).(func(interface{}) json.RawMessage); ok { - r0 = rf(_a0) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(json.RawMessage) - } - } - - var r1 error - if rf, ok := ret.Get(1).(func(interface{}) error); ok { - r1 = rf(_a0) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// ValidateExperimentConfig provides a mock function with given fields: _a0 -func (_m *CustomExperimentManager) ValidateExperimentConfig(_a0 interface{}) error { - ret := _m.Called(_a0) - - var r0 error - if rf, ok := ret.Get(0).(func(interface{}) error); ok { - r0 = rf(_a0) - } else { - r0 = ret.Error(0) - } - - return r0 -} diff --git a/engines/experiment/manager/mocks/ExperimentManager.go b/engines/experiment/manager/mocks/ExperimentManager.go index d6bbbf8c7..a7a9df4ff 100644 --- a/engines/experiment/manager/mocks/ExperimentManager.go +++ b/engines/experiment/manager/mocks/ExperimentManager.go @@ -3,7 +3,9 @@ package mocks import ( - "github.com/gojek/turing/engines/experiment/manager" + json "encoding/json" + + manager "github.com/gojek/turing/engines/experiment/manager" mock "github.com/stretchr/testify/mock" ) @@ -13,7 +15,7 @@ type ExperimentManager struct { } // GetEngineInfo provides a mock function with given fields: -func (_m *ExperimentManager) GetEngineInfo() manager.Engine { +func (_m *ExperimentManager) GetEngineInfo() (manager.Engine, error) { ret := _m.Called() var r0 manager.Engine @@ -23,5 +25,49 @@ func (_m *ExperimentManager) GetEngineInfo() manager.Engine { r0 = ret.Get(0).(manager.Engine) } + var r1 error + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetExperimentRunnerConfig provides a mock function with given fields: cfg +func (_m *ExperimentManager) GetExperimentRunnerConfig(cfg json.RawMessage) (json.RawMessage, error) { + ret := _m.Called(cfg) + + var r0 json.RawMessage + if rf, ok := ret.Get(0).(func(json.RawMessage) json.RawMessage); ok { + r0 = rf(cfg) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(json.RawMessage) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(json.RawMessage) error); ok { + r1 = rf(cfg) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ValidateExperimentConfig provides a mock function with given fields: cfg +func (_m *ExperimentManager) ValidateExperimentConfig(cfg json.RawMessage) error { + ret := _m.Called(cfg) + + var r0 error + if rf, ok := ret.Get(0).(func(json.RawMessage) error); ok { + r0 = rf(cfg) + } else { + r0 = ret.Error(0) + } + return r0 } diff --git a/engines/experiment/manager/mocks/StandardExperimentManager.go b/engines/experiment/manager/mocks/StandardExperimentManager.go index 7c8e89c0b..03255ab36 100644 --- a/engines/experiment/manager/mocks/StandardExperimentManager.go +++ b/engines/experiment/manager/mocks/StandardExperimentManager.go @@ -4,8 +4,8 @@ package mocks import ( json "encoding/json" - "github.com/gojek/turing/engines/experiment/manager" + manager "github.com/gojek/turing/engines/experiment/manager" mock "github.com/stretchr/testify/mock" ) @@ -15,7 +15,7 @@ type StandardExperimentManager struct { } // GetEngineInfo provides a mock function with given fields: -func (_m *StandardExperimentManager) GetEngineInfo() manager.Engine { +func (_m *StandardExperimentManager) GetEngineInfo() (manager.Engine, error) { ret := _m.Called() var r0 manager.Engine @@ -25,16 +25,23 @@ func (_m *StandardExperimentManager) GetEngineInfo() manager.Engine { r0 = ret.Get(0).(manager.Engine) } - return r0 + var r1 error + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 } -// GetExperimentRunnerConfig provides a mock function with given fields: _a0 -func (_m *StandardExperimentManager) GetExperimentRunnerConfig(_a0 manager.TuringExperimentConfig) (json.RawMessage, error) { - ret := _m.Called(_a0) +// GetExperimentRunnerConfig provides a mock function with given fields: cfg +func (_m *StandardExperimentManager) GetExperimentRunnerConfig(cfg json.RawMessage) (json.RawMessage, error) { + ret := _m.Called(cfg) var r0 json.RawMessage - if rf, ok := ret.Get(0).(func(manager.TuringExperimentConfig) json.RawMessage); ok { - r0 = rf(_a0) + if rf, ok := ret.Get(0).(func(json.RawMessage) json.RawMessage); ok { + r0 = rf(cfg) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(json.RawMessage) @@ -42,8 +49,8 @@ func (_m *StandardExperimentManager) GetExperimentRunnerConfig(_a0 manager.Turin } var r1 error - if rf, ok := ret.Get(1).(func(manager.TuringExperimentConfig) error); ok { - r1 = rf(_a0) + if rf, ok := ret.Get(1).(func(json.RawMessage) error); ok { + r1 = rf(cfg) } else { r1 = ret.Error(1) } @@ -52,7 +59,7 @@ func (_m *StandardExperimentManager) GetExperimentRunnerConfig(_a0 manager.Turin } // IsCacheEnabled provides a mock function with given fields: -func (_m *StandardExperimentManager) IsCacheEnabled() bool { +func (_m *StandardExperimentManager) IsCacheEnabled() (bool, error) { ret := _m.Called() var r0 bool @@ -62,7 +69,14 @@ func (_m *StandardExperimentManager) IsCacheEnabled() bool { r0 = ret.Get(0).(bool) } - return r0 + var r1 error + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 } // ListClients provides a mock function with given fields: @@ -180,13 +194,13 @@ func (_m *StandardExperimentManager) ListVariablesForExperiments(_a0 []manager.E return r0, r1 } -// ValidateExperimentConfig provides a mock function with given fields: _a0, _a1 -func (_m *StandardExperimentManager) ValidateExperimentConfig(_a0 *manager.StandardExperimentManagerConfig, _a1 manager.TuringExperimentConfig) error { - ret := _m.Called(_a0, _a1) +// ValidateExperimentConfig provides a mock function with given fields: cfg +func (_m *StandardExperimentManager) ValidateExperimentConfig(cfg json.RawMessage) error { + ret := _m.Called(cfg) var r0 error - if rf, ok := ret.Get(0).(func(*manager.StandardExperimentManagerConfig, manager.TuringExperimentConfig) error); ok { - r0 = rf(_a0, _a1) + if rf, ok := ret.Get(0).(func(json.RawMessage) error); ok { + r0 = rf(cfg) } else { r0 = ret.Error(0) } diff --git a/engines/experiment/pkg/request/request.go b/engines/experiment/pkg/request/request.go index 056b49fa1..4e4ef8c73 100644 --- a/engines/experiment/pkg/request/request.go +++ b/engines/experiment/pkg/request/request.go @@ -1,7 +1,6 @@ package request import ( - "encoding/json" "fmt" "net/http" "strings" @@ -29,20 +28,7 @@ func GetFieldSource(srcString string) (FieldSource, error) { case "payload": return PayloadFieldSource, nil } - return FieldSource(""), fmt.Errorf("Unknown field source %s", srcString) -} - -// UnmarshalJSON unmarshalls the data as a string and then creates the -// appropriate FieldSource -func (f *FieldSource) UnmarshalJSON(data []byte) error { - var s string - var err error - if err = json.Unmarshal(data, &s); err != nil { - return err - } - - *f, err = GetFieldSource(s) - return err + return "", fmt.Errorf("Unknown field source %s", srcString) } // GetValueFromRequest parses the request header / payload to retrieve the value diff --git a/engines/experiment/pkg/request/request_test.go b/engines/experiment/pkg/request/request_test.go index c3da797cb..0cf9b4bd1 100644 --- a/engines/experiment/pkg/request/request_test.go +++ b/engines/experiment/pkg/request/request_test.go @@ -1,7 +1,6 @@ package request_test import ( - "encoding/json" "net/http" "testing" @@ -23,20 +22,6 @@ func TestGetFieldSource(t *testing.T) { assert.Error(t, err) } -func TestUnmarshalJSONFieldSource(t *testing.T) { - var fieldSrc request.FieldSource - // success - err := json.Unmarshal([]byte(`"header"`), &fieldSrc) - assert.Equal(t, request.HeaderFieldSource, fieldSrc) - assert.NoError(t, err) - // unknown string - err = json.Unmarshal([]byte(`"test"`), &fieldSrc) - assert.Error(t, err) - // invalid data - err = json.Unmarshal([]byte(`0`), &fieldSrc) - assert.Error(t, err) -} - func TestGetValueFromRequest(t *testing.T) { tests := map[string]struct { field string diff --git a/engines/experiment/plugin/inproc/manager/manager_test.go b/engines/experiment/plugin/inproc/manager/manager_test.go index c1b323609..f8f7a054c 100644 --- a/engines/experiment/plugin/inproc/manager/manager_test.go +++ b/engines/experiment/plugin/inproc/manager/manager_test.go @@ -8,7 +8,6 @@ import ( "github.com/gojek/turing/engines/experiment/manager" "github.com/gojek/turing/engines/experiment/manager/mocks" managerPlugin "github.com/gojek/turing/engines/experiment/plugin/inproc/manager" - "github.com/stretchr/testify/assert" ) type fakeManager struct { @@ -76,42 +75,3 @@ func TestRegisterAndGet(t *testing.T) { }) } } - -func TestGetStandardExperimentConfig(t *testing.T) { - tests := map[string]struct { - cfg interface{} - expected manager.TuringExperimentConfig - err string - }{ - "failure | invalid json": { - cfg: func() {}, - err: "json: unsupported type: func()", - }, - "failure | invalid standard config": { - cfg: []int{1, 2}, - err: "json: cannot unmarshal array into Go value of type manager.TuringExperimentConfig", - }, - "success": { - cfg: struct { - Client manager.Client `json:"client,omitempty"` - Experiments []manager.Experiment `json:"experiments,omitempty"` - Variables manager.Variables `json:"variables,omitempty"` - Extra int `json:"extra_value"` - }{}, - expected: manager.TuringExperimentConfig{}, - }, - } - - // Run tests - for name, data := range tests { - t.Run(name, func(t *testing.T) { - stdCfg, err := manager.GetStandardExperimentConfig(data.cfg) - if data.err != "" { - assert.EqualError(t, err, data.err) - } else { - assert.NoError(t, err) - assert.Equal(t, data.expected, stdCfg) - } - }) - } -} diff --git a/engines/experiment/plugin/rpc/factory_test.go b/engines/experiment/plugin/rpc/factory_test.go index 861c8bfa9..5bcca9f5a 100644 --- a/engines/experiment/plugin/rpc/factory_test.go +++ b/engines/experiment/plugin/rpc/factory_test.go @@ -78,9 +78,7 @@ func TestEngineFactory_GetExperimentManager(t *testing.T) { cfg: json.RawMessage("{\"key_1\": \"value_1\"}"), mockManager: func(cfg json.RawMessage) interface{} { mockManager := &mocks.ConfigurableExperimentManager{} - mockManager. - On("Configure", cfg). - Return(func() error { return nil }) + mockManager.On("Configure", cfg).Return(nil) return mockManager }, @@ -105,11 +103,7 @@ func TestEngineFactory_GetExperimentManager(t *testing.T) { "failure | failed to configure plugin": { mockManager: func(cfg json.RawMessage) interface{} { mockManager := &mocks.ConfigurableExperimentManager{} - mockManager. - On("Configure", cfg). - Return(func() error { - return errors.New(configError) - }) + mockManager.On("Configure", cfg).Return(errors.New(configError)) return mockManager }, diff --git a/engines/experiment/plugin/rpc/manager/plugin_test.go b/engines/experiment/plugin/rpc/manager/plugin_test.go new file mode 100644 index 000000000..b00a15d40 --- /dev/null +++ b/engines/experiment/plugin/rpc/manager/plugin_test.go @@ -0,0 +1,456 @@ +package manager_test + +import ( + "encoding/json" + "errors" + "fmt" + "testing" + + "github.com/gojek/turing/engines/experiment/manager" + "github.com/stretchr/testify/assert" + + "github.com/gojek/turing/engines/experiment/plugin/rpc" + rpcManager "github.com/gojek/turing/engines/experiment/plugin/rpc/manager" + "github.com/gojek/turing/engines/experiment/plugin/rpc/mocks" + "github.com/hashicorp/go-plugin" +) + +func configuredManagerMock() *mocks.ConfigurableExperimentManager { + mockManager := &mocks.ConfigurableExperimentManager{} + mockManager.On("Configure", json.RawMessage(nil)).Return(nil) + + return mockManager +} + +func configuredStandardManagerMock() *mocks.ConfigurableStandardExperimentManager { + mockManager := &mocks.ConfigurableStandardExperimentManager{} + mockManager.On("Configure", json.RawMessage(nil)).Return(nil) + + return mockManager +} + +func withExperimentManager( + t *testing.T, + impl rpcManager.ConfigurableExperimentManager, + testFn func(em manager.ExperimentManager, err error), +) { + plugins := map[string]plugin.Plugin{ + rpc.ManagerPluginIdentifier: &rpcManager.ExperimentManagerPlugin{ + Impl: impl, + }, + } + + client, _ := plugin.TestPluginRPCConn(t, plugins, nil) + + factory := &rpc.EngineFactory{ + Client: client, + EngineConfig: nil, + } + + testFn(factory.GetExperimentManager()) +} + +func TestExperimentManagerPlugin_Configure(t *testing.T) { + suite := map[string]struct { + err error + }{ + "success": {}, + "failure | configuration error": { + err: errors.New("failed to configure plugin"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := &mocks.ConfigurableExperimentManager{} + mockManager.On("Configure", json.RawMessage(nil)).Return(tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, err error) { + if tt.err != nil { + assert.Nil(t, em) + assert.EqualError(t, err, + fmt.Sprintf("failed to configure \"experiment_manager\" plugin instance: %v", tt.err)) + } else { + assert.NoError(t, err) + assert.NotNil(t, em) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_GetEngineInfo(t *testing.T) { + suite := map[string]struct { + expected manager.Engine + err error + }{ + "success": { + expected: manager.Engine{ + Name: "standard", + DisplayName: "Standard Manager", + Type: manager.StandardExperimentManagerType, + StandardExperimentManagerConfig: &manager.StandardExperimentManagerConfig{ + ClientSelectionEnabled: true, + ExperimentSelectionEnabled: true, + HomePageURL: "http://example.com", + }, + }, + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredManagerMock() + mockManager.On("GetEngineInfo").Return(tt.expected, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.GetEngineInfo() + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_ValidateExperimentConfig(t *testing.T) { + suite := map[string]struct { + experimentConfig json.RawMessage + err error + }{ + "success | validation passed": { + experimentConfig: json.RawMessage(`{ + "my_config": "my_value" + `), + }, + "failure | validation failed": { + experimentConfig: json.RawMessage(`[1, 2]`), + err: errors.New("validation failed"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredManagerMock() + mockManager.On("ValidateExperimentConfig", tt.experimentConfig).Return(tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + err := em.ValidateExperimentConfig(tt.experimentConfig) + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_GetExperimentRunnerConfig(t *testing.T) { + suite := map[string]struct { + experimentConfig json.RawMessage + expected json.RawMessage + err error + }{ + "success | custom experiment config": { + experimentConfig: json.RawMessage(`{ + "client": { + "id": "client-id" + } + `), + expected: json.RawMessage(`{}`), + }, + "failure": { + experimentConfig: json.RawMessage("unexpected config"), + err: errors.New("failed to retrieve runner's config"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredManagerMock() + mockManager.On("GetExperimentRunnerConfig", tt.experimentConfig).Return(tt.expected, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.GetExperimentRunnerConfig(tt.experimentConfig) + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_IsCacheEnabled(t *testing.T) { + suite := map[string]struct { + cacheEnabled bool + err error + }{ + "success | enabled": { + cacheEnabled: true, + }, + "failure | error": { + err: errors.New("failure"), + }, + "success | disabled": { + cacheEnabled: false, + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredStandardManagerMock() + mockManager.On("IsCacheEnabled").Return(tt.cacheEnabled, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.(manager.StandardExperimentManager).IsCacheEnabled() + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + assert.False(t, actual) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.cacheEnabled, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_ListClients(t *testing.T) { + suite := map[string]struct { + expected []manager.Client + err error + }{ + "success": { + expected: []manager.Client{ + { + ID: "client-1", + Username: "username-1", + }, + }, + }, + "failure": { + err: errors.New("failed to fetch clients"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredStandardManagerMock() + mockManager.On("ListClients").Return(tt.expected, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.(manager.StandardExperimentManager).ListClients() + + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_ListExperiments(t *testing.T) { + suite := map[string]struct { + expected []manager.Experiment + err error + }{ + "success": { + expected: []manager.Experiment{ + { + ID: "123-456-789", + Name: "experiment-01", + ClientID: "client-01", + }, + }, + }, + "failure": { + err: errors.New("failed to fetch experiments"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredStandardManagerMock() + mockManager.On("ListExperiments").Return(tt.expected, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.(manager.StandardExperimentManager).ListExperiments() + + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_ListExperimentsForClient(t *testing.T) { + suite := map[string]struct { + client manager.Client + expected []manager.Experiment + err error + }{ + "success": { + client: manager.Client{ + ID: "client-02", + }, + expected: []manager.Experiment{ + { + Name: "experiment-02", + ClientID: "client-02", + }, + }, + }, + "failure": { + err: errors.New("failed to fetch experiments for client"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredStandardManagerMock() + mockManager.On("ListExperimentsForClient", tt.client).Return(tt.expected, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.(manager.StandardExperimentManager).ListExperimentsForClient(tt.client) + + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_ListVariablesForClient(t *testing.T) { + suite := map[string]struct { + client manager.Client + expected []manager.Variable + err error + }{ + "success": { + client: manager.Client{ + ID: "client-03", + }, + expected: []manager.Variable{ + { + Name: "sessionId", + Required: false, + Type: manager.FilterVariableType, + }, + { + Name: "customerType", + Required: true, + Type: manager.UnitVariableType, + }, + }, + }, + "failure": { + client: manager.Client{ + ID: "unknown", + }, + err: errors.New("failed to fetch variables for client"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredStandardManagerMock() + mockManager.On("ListVariablesForClient", tt.client).Return(tt.expected, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.(manager.StandardExperimentManager).ListVariablesForClient(tt.client) + + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} + +func TestExperimentManagerPlugin_ListVariablesForExperiments(t *testing.T) { + suite := map[string]struct { + experiments []manager.Experiment + expected map[string][]manager.Variable + err error + }{ + "success": { + experiments: []manager.Experiment{ + { + Name: "experiment-04", + }, + }, + expected: map[string][]manager.Variable{ + "experiment-04": { + { + Name: "customerType", + Required: true, + Type: manager.UnitVariableType, + }, + }, + }, + }, + "failure": { + experiments: []manager.Experiment(nil), + err: errors.New("failed to fetch variables for client"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockManager := configuredStandardManagerMock() + mockManager.On("ListVariablesForExperiments", tt.experiments).Return(tt.expected, tt.err) + + withExperimentManager(t, mockManager, func(em manager.ExperimentManager, _ error) { + actual, err := em.(manager.StandardExperimentManager).ListVariablesForExperiments(tt.experiments) + + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + + mockManager.AssertExpectations(t) + }) + } +} diff --git a/engines/experiment/plugin/rpc/manager/rpc.go b/engines/experiment/plugin/rpc/manager/rpc.go deleted file mode 100644 index 124304b5f..000000000 --- a/engines/experiment/plugin/rpc/manager/rpc.go +++ /dev/null @@ -1,45 +0,0 @@ -package manager - -import ( - "encoding/json" - "fmt" - - "github.com/gojek/turing/engines/experiment/manager" - "github.com/gojek/turing/engines/experiment/plugin/rpc/shared" -) - -// rpcClient implements ConfigurableExperimentManager interface -type rpcClient struct { - shared.RPCClient -} - -func (c *rpcClient) Configure(cfg json.RawMessage) error { - return c.Call("Plugin.Configure", cfg, new(interface{})) -} - -func (c *rpcClient) GetEngineInfo() manager.Engine { - resp := manager.Engine{} - err := c.Call("Plugin.GetEngineInfo", new(interface{}), &resp) - if err != nil { - // err should be propagated upstream, but it's currently not - // possible as GetEngineInfo() on the manager.ExperimentManager - // interface doesn't return errors - fmt.Printf("plugin errors: %v", err) - } - - return resp -} - -// rpcServer serves the implementation of a ConfigurableExperimentManager -type rpcServer struct { - Impl ConfigurableExperimentManager -} - -func (s *rpcServer) Configure(cfg json.RawMessage, _ *interface{}) (err error) { - return s.Impl.Configure(cfg) -} - -func (s *rpcServer) GetEngineInfo(_ interface{}, resp *manager.Engine) error { - *resp = s.Impl.GetEngineInfo() - return nil -} diff --git a/engines/experiment/plugin/rpc/manager/rpc_client.go b/engines/experiment/plugin/rpc/manager/rpc_client.go new file mode 100644 index 000000000..bfde986af --- /dev/null +++ b/engines/experiment/plugin/rpc/manager/rpc_client.go @@ -0,0 +1,67 @@ +package manager + +import ( + "encoding/json" + + "github.com/gojek/turing/engines/experiment/manager" + "github.com/gojek/turing/engines/experiment/plugin/rpc/shared" +) + +// rpcClient implements ConfigurableExperimentManager interface +type rpcClient struct { + shared.RPCClient +} + +func (c *rpcClient) Configure(cfg json.RawMessage) error { + return c.Call("Plugin.Configure", cfg, new(interface{})) +} + +func (c *rpcClient) GetEngineInfo() (resp manager.Engine, err error) { + err = c.Call("Plugin.GetEngineInfo", new(interface{}), &resp) + return +} + +func (c *rpcClient) ValidateExperimentConfig(cfg json.RawMessage) error { + return c.Call("Plugin.ValidateExperimentConfig", cfg, new(interface{})) +} + +func (c *rpcClient) GetExperimentRunnerConfig(cfg json.RawMessage) (resp json.RawMessage, err error) { + err = c.Call("Plugin.GetExperimentRunnerConfig", cfg, &resp) + return +} + +func (c *rpcClient) IsCacheEnabled() (resp bool, err error) { + err = c.Call("Plugin.IsCacheEnabled", new(interface{}), &resp) + return +} + +func (c *rpcClient) ListClients() (resp []manager.Client, err error) { + resp = []manager.Client{} + err = c.Call("Plugin.ListClients", new(interface{}), &resp) + return +} + +func (c *rpcClient) ListExperiments() (resp []manager.Experiment, err error) { + resp = []manager.Experiment{} + err = c.Call("Plugin.ListExperiments", new(interface{}), &resp) + return +} + +func (c *rpcClient) ListExperimentsForClient(client manager.Client) (resp []manager.Experiment, err error) { + resp = []manager.Experiment{} + err = c.Call("Plugin.ListExperimentsForClient", client, &resp) + return +} + +func (c *rpcClient) ListVariablesForClient(client manager.Client) (resp []manager.Variable, err error) { + resp = []manager.Variable{} + err = c.Call("Plugin.ListVariablesForClient", client, &resp) + return +} + +func (c *rpcClient) ListVariablesForExperiments( + experiments []manager.Experiment, +) (resp map[string][]manager.Variable, err error) { + err = c.Call("Plugin.ListVariablesForExperiments", experiments, &resp) + return +} diff --git a/engines/experiment/plugin/rpc/manager/rpc_client_test.go b/engines/experiment/plugin/rpc/manager/rpc_client_test.go new file mode 100644 index 000000000..011dcf52d --- /dev/null +++ b/engines/experiment/plugin/rpc/manager/rpc_client_test.go @@ -0,0 +1,92 @@ +package manager + +import ( + "encoding/json" + "errors" + "testing" + + "github.com/gojek/turing/engines/experiment/manager" + "github.com/gojek/turing/engines/experiment/plugin/rpc/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func TestRpcClient_Configure(t *testing.T) { + suite := map[string]struct { + cfg json.RawMessage + err string + }{ + "success": { + cfg: json.RawMessage("{\"my_config\": \"my_value\"}"), + }, + "failure": { + cfg: json.RawMessage("{\"my_config\": \"my_value\"}"), + err: "failed to configure experiment manager plugin", + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockClient := &mocks.RPCClient{} + mockClient.On("Call", "Plugin.Configure", tt.cfg, mock.Anything).Return( + func() error { + if tt.err != "" { + return errors.New(tt.err) + } + return nil + }) + + rpcClient := rpcClient{RPCClient: mockClient} + + err := rpcClient.Configure(tt.cfg) + if tt.err != "" { + assert.EqualError(t, err, tt.err) + } else { + assert.NoError(t, err) + } + + mockClient.AssertExpectations(t) + }) + } +} + +func TestRpcClient_GetEngineInfo(t *testing.T) { + suite := map[string]struct { + expected manager.Engine + err error + }{ + "success | get engine info": { + expected: manager.Engine{ + Name: "engine-1", + Type: manager.StandardExperimentManagerType, + }, + }, + "failure | get engine info": { + err: errors.New("something's wrong"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + mockClient := &mocks.RPCClient{} + mockClient. + On("Call", "Plugin.GetEngineInfo", mock.Anything, mock.AnythingOfType("*manager.Engine")). + Run(func(args mock.Arguments) { + resp := args.Get(2).(*manager.Engine) + *resp = tt.expected + }). + Return(tt.err) + + rpcClient := rpcClient{RPCClient: mockClient} + + actual, _ := rpcClient.GetEngineInfo() + if tt.err != nil { + empty := manager.Engine{} + assert.Equal(t, actual, empty) + } else { + assert.Equal(t, actual, tt.expected) + } + mockClient.AssertExpectations(t) + }) + } +} diff --git a/engines/experiment/plugin/rpc/manager/rpc_server.go b/engines/experiment/plugin/rpc/manager/rpc_server.go new file mode 100644 index 000000000..e0f8c7c91 --- /dev/null +++ b/engines/experiment/plugin/rpc/manager/rpc_server.go @@ -0,0 +1,87 @@ +package manager + +import ( + "encoding/json" + "errors" + + "github.com/gojek/turing/engines/experiment/manager" +) + +// rpcServer serves the implementation of a ConfigurableExperimentManager +type rpcServer struct { + Impl ConfigurableExperimentManager +} + +func (s *rpcServer) Configure(cfg json.RawMessage, _ *interface{}) (err error) { + return s.Impl.Configure(cfg) +} + +func (s *rpcServer) GetEngineInfo(_ interface{}, resp *manager.Engine) (err error) { + *resp, err = s.Impl.GetEngineInfo() + return +} + +func (s *rpcServer) ValidateExperimentConfig(cfg json.RawMessage, _ *interface{}) (err error) { + return s.Impl.ValidateExperimentConfig(cfg) +} + +func (s *rpcServer) GetExperimentRunnerConfig(inConfig json.RawMessage, outConfig *json.RawMessage) (err error) { + *outConfig, err = s.Impl.GetExperimentRunnerConfig(inConfig) + return +} + +// Methods of manager.StandardExperimentManager are served below this line + +func (s *rpcServer) IsCacheEnabled(_ interface{}, resp *bool) error { + return s.asStandardManager(func(sm manager.StandardExperimentManager) (err error) { + *resp, err = sm.IsCacheEnabled() + return + }) +} + +func (s *rpcServer) ListClients(_ interface{}, resp *[]manager.Client) error { + return s.asStandardManager(func(sm manager.StandardExperimentManager) (err error) { + *resp, err = sm.ListClients() + return + }) +} + +func (s *rpcServer) ListExperiments(_ interface{}, resp *[]manager.Experiment) error { + return s.asStandardManager(func(sm manager.StandardExperimentManager) (err error) { + *resp, err = sm.ListExperiments() + return + }) +} + +func (s *rpcServer) ListExperimentsForClient(client manager.Client, resp *[]manager.Experiment) error { + return s.asStandardManager(func(sm manager.StandardExperimentManager) (err error) { + *resp, err = sm.ListExperimentsForClient(client) + return + }) +} + +func (s *rpcServer) ListVariablesForClient(client manager.Client, resp *[]manager.Variable) error { + return s.asStandardManager(func(sm manager.StandardExperimentManager) (err error) { + *resp, err = sm.ListVariablesForClient(client) + return + }) +} + +func (s *rpcServer) ListVariablesForExperiments( + experiments []manager.Experiment, + resp *map[string][]manager.Variable, +) error { + return s.asStandardManager(func(sm manager.StandardExperimentManager) (err error) { + *resp, err = sm.ListVariablesForExperiments(experiments) + return + }) +} + +func (s *rpcServer) asStandardManager(fn func(manager.StandardExperimentManager) error) error { + standardManager, ok := s.Impl.(manager.StandardExperimentManager) + if !ok { + return errors.New("not implemented") + } + + return fn(standardManager) +} diff --git a/engines/experiment/plugin/rpc/manager/rpc_test.go b/engines/experiment/plugin/rpc/manager/rpc_server_test.go similarity index 51% rename from engines/experiment/plugin/rpc/manager/rpc_test.go rename to engines/experiment/plugin/rpc/manager/rpc_server_test.go index 80af30093..f28febc4b 100644 --- a/engines/experiment/plugin/rpc/manager/rpc_test.go +++ b/engines/experiment/plugin/rpc/manager/rpc_server_test.go @@ -11,91 +11,6 @@ import ( "github.com/stretchr/testify/mock" ) -func TestRpcClient_Configure(t *testing.T) { - suite := map[string]struct { - cfg json.RawMessage - err string - }{ - "success": { - cfg: json.RawMessage("{\"my_config\": \"my_value\"}"), - }, - "failure": { - cfg: json.RawMessage("{\"my_config\": \"my_value\"}"), - err: "failed to configure experiment manager plugin", - }, - } - - for name, tt := range suite { - t.Run(name, func(t *testing.T) { - mockClient := &mocks.RPCClient{} - mockClient.On("Call", "Plugin.Configure", tt.cfg, mock.Anything).Return( - func() error { - if tt.err != "" { - return errors.New(tt.err) - } - return nil - }) - - rpcClient := rpcClient{RPCClient: mockClient} - - err := rpcClient.Configure(tt.cfg) - if tt.err != "" { - assert.EqualError(t, err, tt.err) - } else { - assert.NoError(t, err) - } - - mockClient.AssertExpectations(t) - }) - } -} - -func TestRpcClient_GetEngineInfo(t *testing.T) { - suite := map[string]struct { - expected manager.Engine - err string - }{ - "success | get engine info": { - expected: manager.Engine{ - Name: "engine-1", - Type: manager.StandardExperimentManagerType, - }, - }, - "failure | get engine info": { - err: "something's wrong", - }, - } - - for name, tt := range suite { - t.Run(name, func(t *testing.T) { - mockClient := &mocks.RPCClient{} - mockClient. - On("Call", "Plugin.GetEngineInfo", mock.Anything, mock.AnythingOfType("*manager.Engine")). - Run(func(args mock.Arguments) { - resp := args.Get(2).(*manager.Engine) - *resp = tt.expected - }). - Return(func() error { - if tt.err != "" { - return errors.New(tt.err) - } - return nil - }) - - rpcClient := rpcClient{RPCClient: mockClient} - - actual := rpcClient.GetEngineInfo() - if tt.err != "" { - empty := manager.Engine{} - assert.Equal(t, actual, empty) - } else { - assert.Equal(t, actual, tt.expected) - } - mockClient.AssertExpectations(t) - }) - } -} - func TestRpcServer_Configure(t *testing.T) { suite := map[string]struct { cfg json.RawMessage @@ -135,7 +50,7 @@ func TestRpcServer_Configure(t *testing.T) { func TestRpcServer_GetEngineInfo(t *testing.T) { suite := map[string]struct { expected manager.Engine - err string + err error }{ "success | get engine info": { expected: manager.Engine{ @@ -148,15 +63,14 @@ func TestRpcServer_GetEngineInfo(t *testing.T) { for name, tt := range suite { t.Run(name, func(t *testing.T) { mockManager := &mocks.ConfigurableExperimentManager{} - mockManager.On("GetEngineInfo", mock.Anything). - Return(tt.expected) + mockManager.On("GetEngineInfo", mock.Anything).Return(tt.expected, tt.err) rpcServer := &rpcServer{mockManager} var actual manager.Engine err := rpcServer.GetEngineInfo(nil, &actual) - if tt.err != "" { - assert.EqualError(t, err, tt.err) + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) } else { assert.NoError(t, err) assert.Equal(t, tt.expected, actual) @@ -165,3 +79,43 @@ func TestRpcServer_GetEngineInfo(t *testing.T) { }) } } + +func TestRpcServer_IsCacheEnabled(t *testing.T) { + suite := map[string]struct { + managerMock func(expected bool, err error) ConfigurableExperimentManager + expected bool + err error + }{ + "success": { + managerMock: func(expected bool, err error) ConfigurableExperimentManager { + mm := &mocks.ConfigurableStandardExperimentManager{} + mm.On("IsCacheEnabled").Return(expected, err) + + return mm + }, + expected: true, + }, + "failure": { + managerMock: func(bool, error) ConfigurableExperimentManager { + return &mocks.ConfigurableExperimentManager{} + }, + err: errors.New("not implemented"), + }, + } + + for name, tt := range suite { + t.Run(name, func(t *testing.T) { + rpcServer := &rpcServer{tt.managerMock(tt.expected, tt.err)} + + var actual bool + err := rpcServer.IsCacheEnabled(nil, &actual) + + if tt.err != nil { + assert.EqualError(t, err, tt.err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, actual) + } + }) + } +} diff --git a/engines/experiment/plugin/rpc/mocks/experiment_manager.go b/engines/experiment/plugin/rpc/mocks/experiment_manager.go index bec122be6..0ad637994 100644 --- a/engines/experiment/plugin/rpc/mocks/experiment_manager.go +++ b/engines/experiment/plugin/rpc/mocks/experiment_manager.go @@ -17,7 +17,24 @@ func (_m *ConfigurableExperimentManager) Configure(cfg json.RawMessage) error { if rf, ok := ret.Get(0).(func() error); ok { r0 = rf() } else { - r0 = ret.Get(0).(error) + r0 = ret.Error(0) + } + + return r0 +} + +type ConfigurableStandardExperimentManager struct { + mocks.StandardExperimentManager +} + +func (_m *ConfigurableStandardExperimentManager) Configure(cfg json.RawMessage) error { + ret := _m.Called(cfg) + + var r0 error + if rf, ok := ret.Get(0).(func() error); ok { + r0 = rf() + } else { + r0 = ret.Error(0) } return r0 diff --git a/engines/experiment/plugin/rpc/mocks/rpc_client.go b/engines/experiment/plugin/rpc/mocks/rpc_client.go index 4a474f824..d270ebdb2 100644 --- a/engines/experiment/plugin/rpc/mocks/rpc_client.go +++ b/engines/experiment/plugin/rpc/mocks/rpc_client.go @@ -13,7 +13,7 @@ func (_m *RPCClient) Call(serviceMethod string, args interface{}, reply interfac if rf, ok := ret.Get(0).(func() error); ok { r0 = rf() } else { - r0 = ret.Get(0).(error) + r0 = ret.Error(0) } return r0 diff --git a/engines/router/missionctl/fiberapi/fan_in.go b/engines/router/missionctl/fiberapi/fan_in.go index 303edefef..d67a9a5df 100644 --- a/engines/router/missionctl/fiberapi/fan_in.go +++ b/engines/router/missionctl/fiberapi/fan_in.go @@ -45,7 +45,7 @@ func (fanIn *EnsemblingFanIn) Initialize(properties json.RawMessage) error { // Aggregate requests for the treatment parameters from the configured experiment engine, // collects the results from the fanout, dispatches the combined data to the configured -// ensembling endpoint and returs the result +// ensembling endpoint and returns the result func (fanIn *EnsemblingFanIn) Aggregate( ctx context.Context, req fiber.Request,