refact: Updated polling_manager implementation and unit tests. #224
Conversation
|
|
||
| // SyncConfig gets current datafile and updates projectConfig | ||
| func (cm *PollingProjectConfigManager) SyncConfig(datafile []byte) { | ||
| func (cm *PollingProjectConfigManager) SyncConfig() { |
pkg/config/polling_manager.go
Outdated
| if projectConfig, _ := datafileprojectconfig.NewDatafileProjectConfig(initDatafile); projectConfig != nil { | ||
| _ = pollingProjectConfigManager.setConfig(projectConfig) | ||
| } | ||
| pollingProjectConfigManager.notificationCenter = registry.GetNotificationCenter(sdkKey) |
There was a problem hiding this comment.
Please add comment, notificationcenter is set later to avoid hard-coded config update notification.
pkg/config/polling_manager.go
Outdated
| cm.err = nil | ||
| cm.configLock.Unlock() | ||
|
|
||
| if cm.notificationCenter != nil { |
There was a problem hiding this comment.
I think we should separate notificationCenter in a new method.
| func assertPeriodically(t *testing.T, evaluationMethod func() bool) { | ||
| assert.Eventually(t, func() bool { | ||
| return evaluationMethod() | ||
| }, 500*time.Millisecond, 110*time.Millisecond) |
There was a problem hiding this comment.
I think no issue to execute for 1 second and reduce recurring time to 50ms
There was a problem hiding this comment.
This causes issues in tests where we expect no changes in config after second poll. In such cases, 50ms returns true at 150ms, before the second polling is done which will be at 200ms.
pkg/config/polling_manager_test.go
Outdated
| eg.Go(configManager.Start) | ||
| evaluationMethod := func() bool { | ||
| actual, _ := configManager.GetConfig() | ||
| return reflect.DeepEqual(projectConfig, actual) |
There was a problem hiding this comment.
Please use actual.GetRevision() == hardcodedRevision.
pkg/config/polling_manager_test.go
Outdated
| eg.Go(configManager.Start) | ||
| evaluationMethod := func() bool { | ||
| actual, _ := configManager.GetConfig() | ||
| return reflect.DeepEqual(projectConfig, actual) |
There was a problem hiding this comment.
Please use only revision to identify config version.
pkg/config/polling_manager_test.go
Outdated
| eg.TerminateAndWait() | ||
| } | ||
|
|
||
| func TestSyncConfig(t *testing.T) { |
There was a problem hiding this comment.
What you are trying to test, please add as a comment. TestSyncConfig is very general name.
pkg/config/polling_manager_test.go
Outdated
| // poll after 100ms | ||
| eg.Go(configManager.Start) | ||
| evaluationMethod := func() bool { | ||
| _, err := configManager.GetConfig() |
|
|
||
| func TestNewPollingProjectConfigManagerWithSimilarDatafileRevisions(t *testing.T) { | ||
| mockDatafile1 := []byte(`{"revision":"42","botFiltering":true}`) | ||
| mockDatafile2 := []byte(`{"revision":"42","botFiltering":false}`) |
There was a problem hiding this comment.
botFiltering is false Please make it same.
There was a problem hiding this comment.
We need the difference in datafiles to make sure the original datafile wasn't changed because of same revision.
| eg.TerminateAndWait() | ||
| } | ||
|
|
||
| func TestNewPollingProjectConfigManagerWithSimilarDatafileRevisions(t *testing.T) { |
There was a problem hiding this comment.
Can you please assert notification, it shouldn't be sent. request assertion should be true.
yasirfolio3
changed the title
| eg.TerminateAndWait() | ||
| } | ||
|
|
||
| func TestNewAsyncPollingProjectConfigManagerWithSimilarDatafileRevisions(t *testing.T) { |
There was a problem hiding this comment.
what's here you want to test, please add in the comments.
| asyncConfigManager := NewAsyncPollingProjectConfigManager(sdkKey, WithRequester(mockRequester), WithInitialDatafile(mockDatafile1), WithPollingInterval(100*time.Millisecond)) | ||
|
|
||
| var numberOfCalls uint64 = 0 | ||
| callback := func(notification notification.ProjectConfigUpdateNotification) { |
There was a problem hiding this comment.
if we have similar datafile revision, then how it's gonna be called. please explain the reason to declare here.
pkg/config/polling_manager_test.go
Outdated
|
|
||
| // poll after 100ms | ||
| eg.Go(asyncConfigManager.Start) | ||
| evaluationMethod := func() bool { |
There was a problem hiding this comment.
I think this is not best fit here, just use sleep
pkg/config/polling_manager_test.go
Outdated
| assert.NotNil(t, actual) | ||
| assert.Equal(t, projectConfig1, actual) | ||
| // poll after 100ms to check no changes were made to the previous config because of 304 error code (first poll) | ||
| eg.Go(configManager.Start) |
There was a problem hiding this comment.
Don't use Start, use SyncConfig
pkg/config/polling_manager_test.go
Outdated
| actual, err = configManager.GetConfig() | ||
| assert.Equal(t, projectConfig2, actual) | ||
| // poll after 100ms | ||
| eg.Go(configManager.Start) |
There was a problem hiding this comment.
Start immediately while you are instantiating object.
pkg/config/polling_manager_test.go
Outdated
| assert.Equal(t, projectConfig1, actual) | ||
|
|
||
| // poll after 100ms | ||
| eg.Go(asyncConfigManager.Start) |
There was a problem hiding this comment.
don't poll. just use SyncConfig
pawels-optimizely
left a comment
pawels-optimizely
left a comment
There was a problem hiding this comment.
please see my comments
pkg/config/polling_manager.go
Outdated
| return cm.projectConfig, nil | ||
| } | ||
|
|
||
| func (cm *PollingProjectConfigManager) setConfig(projectConfig ProjectConfig) error { |
There was a problem hiding this comment.
I would remove locking mechanism here and minimize the number of calls to locking.
of course cm.sendConfigUpdateNotification() will need to be called in SyncConfig then
pkg/config/polling_manager.go
Outdated
| cm.setConfig(projectConfig) | ||
| cm.err = nil | ||
| } else { | ||
| cm.err = err |
There was a problem hiding this comment.
I think , to be safe, we need to lock cm.err too.
pkg/config/polling_manager.go
Outdated
| cmLogger.Warning("Problem with sending notification") | ||
| } | ||
| } | ||
| cm.sendConfigUpdateNotification() |
There was a problem hiding this comment.
I think cm.setConfig() should still return an error, and we need to sendConfigUpdateNotification only when error is not nil
pawels-optimizely
left a comment
pawels-optimizely
left a comment
There was a problem hiding this comment.
it looks much cleaner, thank you for the changes, I put 2 small comments which I think need to be addressed. otherwise lgtm
pkg/config/polling_manager.go
Outdated
| cmLogger.Debug(fmt.Sprintf("New datafile set with revision: %s. Old revision: %s", projectConfig.GetRevision(), previousRevision)) | ||
| cm.sendConfigUpdateNotification() | ||
| } | ||
| closeMutex(err) |
There was a problem hiding this comment.
closing mutex needs to happen before sending notification.
pawels-optimizely
left a comment
pawels-optimizely
left a comment
There was a problem hiding this comment.
closing mutex needs to happen before sending notification.
otherwise looks good
Summary