[FSSDK-11003] review update

Author: junaed-optimizely

lib/event_processor/batch_event_processor.spec.ts

@@ -266,10 +266,12 @@ describe('QueueingEventProcessor', async () => {
       mockDispatch.mockResolvedValue({});
 
       const dispatchRepeater = getMockRepeater();
+      const failedEventRepeater = getMockRepeater();
 
       const processor = new BatchEventProcessor({
         eventDispatcher,
         dispatchRepeater,
+        failedEventRepeater,
         batchSize: 100,
       });
 
@@ -285,6 +287,11 @@ describe('QueueingEventProcessor', async () => {
       expect(eventDispatcher.dispatchEvent).toHaveBeenCalledTimes(1);
       expect(eventDispatcher.dispatchEvent.mock.calls[0][0]).toEqual(buildLogEvent(events));
       expect(dispatchRepeater.reset).toHaveBeenCalledTimes(1);
+      expect(dispatchRepeater.start).not.toHaveBeenCalled();
+      expect(failedEventRepeater.start).not.toHaveBeenCalled();
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore
+      expect(processor.retryConfig?.maxRetries).toEqual(5);
     });
 
     it('should store the event in the eventStore with increasing ids', async () => {

lib/event_processor/batch_event_processor.ts

@@ -247,24 +247,30 @@ export class BatchEventProcessor extends BaseService implements EventProcessor {
     if (!this.isNew()) {
       return;
     }
-    if(this.disposable) {
-      this.batchSize = 1;
-      this.retryConfig = {
-        maxRetries: Math.min(this.retryConfig?.maxRetries ?? 5, 5),
-        backoffProvider:
-          this.retryConfig?.backoffProvider ||
-          (() => new ExponentialBackoff(DEFAULT_MIN_BACKOFF, DEFAULT_MAX_BACKOFF, 500)),
-      };
-    }
+
     super.start();
     this.state = ServiceState.Running;
-    this.dispatchRepeater.start();
-    this.failedEventRepeater?.start();
+    
+    if(!this.disposable) {
+      this.dispatchRepeater.start();
+      this.failedEventRepeater?.start();
+    }
 
     this.retryFailedEvents();
     this.startPromise.resolve();
   }
 
+  makeDisposable(): void {
+    super.makeDisposable();
+    this.batchSize = 1;
+    this.retryConfig = {
+      maxRetries: Math.min(this.retryConfig?.maxRetries ?? 5, 5),
+      backoffProvider:
+        this.retryConfig?.backoffProvider ||
+        (() => new ExponentialBackoff(DEFAULT_MIN_BACKOFF, DEFAULT_MAX_BACKOFF, 500)),
+    }
+  }
+
   stop(): void {
     if (this.isDone()) {
       return;

lib/odp/event_manager/odp_event_manager.spec.ts

@@ -211,7 +211,7 @@ describe('DefaultOdpEventManager', () => {
     const apiManager = getMockApiManager();
     const repeater = getMockRepeater()
     apiManager.sendEvents.mockResolvedValue({ statusCode: 200 });
-    // spy on the flush method
+
     const odpEventManager = new DefaultOdpEventManager({
       repeater, 
       apiManager: apiManager,
@@ -238,6 +238,7 @@ describe('DefaultOdpEventManager', () => {
     expect(apiManager.sendEvents).toHaveBeenCalledTimes(1);
     expect(apiManager.sendEvents).toHaveBeenNthCalledWith(1, config, [event]);
     expect(repeater.reset).toHaveBeenCalledTimes(1);
+    expect(repeater.start).not.toHaveBeenCalled();
   })
 
   it('drops events and logs if the state is not running', async () => {

lib/odp/event_manager/odp_event_manager.ts

@@ -105,20 +105,22 @@ export class DefaultOdpEventManager extends BaseService implements OdpEventManag
     if (!this.isNew) {
       return;
     }
-    // Override for disposable event manager 
-    if(this.disposable) {
-      this.retryConfig.maxRetries = Math.min(this.retryConfig.maxRetries, 5);
-      this.batchSize = 1
-    }
 
     super.start();
+
     if (this.odpIntegrationConfig) {
       this.goToRunningState();
     } else {
       this.state = ServiceState.Starting;
     }
   }
 
+  makeDisposable(): void {
+    super.makeDisposable();
+    this.retryConfig.maxRetries = Math.min(this.retryConfig.maxRetries, 5);
+    this.batchSize = 1;
+  }
+
   updateConfig(odpIntegrationConfig: OdpIntegrationConfig): void {
     if (this.isDone()) {
       return;

lib/odp/odp_manager.spec.ts

@@ -697,5 +697,19 @@ describe('DefaultOdpManager', () => {
     eventManagerTerminatedPromise.reject(new Error(FAILED_TO_STOP));
     await expect(odpManager.onTerminated()).rejects.toThrow();
   });
+
+  it('should call makeDisposable() on eventManager when makeDisposable() is called on odpManager', async () => {
+    const eventManager = getMockOdpEventManager();
+    const segmentManager = getMockOdpSegmentManager();
+
+    const odpManager = new DefaultOdpManager({
+      segmentManager,
+      eventManager,
+    });
+
+    odpManager.makeDisposable();
+
+    expect(eventManager.makeDisposable).toHaveBeenCalled();
+  })
 });
 

lib/odp/odp_manager.ts

@@ -90,10 +90,6 @@ export class DefaultOdpManager extends BaseService implements OdpManager {
     if (!this.isNew()) {
       return;
     }
-    
-    if(this.disposable) {
-      this.eventManager.makeDisposable();
-    }
 
     this.state = ServiceState.Starting;
 
@@ -112,6 +108,11 @@ export class DefaultOdpManager extends BaseService implements OdpManager {
       });
   }
 
+  makeDisposable(): void {
+    super.makeDisposable();
+    this.eventManager.makeDisposable();
+  }
+
   private handleStartSuccess() {
     if (this.isDone()) {
       return;

lib/optimizely/index.spec.ts

@@ -22,6 +22,7 @@ import testData from '../tests/test_data';
 import { getForwardingEventProcessor } from '../event_processor/forwarding_event_processor';
 import { createProjectConfig } from '../project_config/project_config';
 import { getMockLogger } from '../tests/mock/mock_logger';
+import { createOdpManager } from '../odp/odp_manager_factory.node';
 
 describe('Optimizely', () => {
   const errorHandler = { handleError: function() {} };
@@ -31,36 +32,34 @@ describe('Optimizely', () => {
   };
 
   const eventProcessor = getForwardingEventProcessor(eventDispatcher);
-
+  const odpManager = createOdpManager({});
   const logger = getMockLogger();
-
   const notificationCenter = createNotificationCenter({ logger, errorHandler });
 
-  it('should pass disposable option to the project config manager', () => {
+  it('should pass disposable options to the respective services', () => {
     const projectConfigManager = getMockProjectConfigManager({
       initConfig: createProjectConfig(testData.getTestProjectConfig()),
     });
-    
+
     vi.spyOn(projectConfigManager, 'makeDisposable');
+    vi.spyOn(eventProcessor, 'makeDisposable');
+    vi.spyOn(odpManager, 'makeDisposable');
 
-    const instance = new Optimizely({
+    new Optimizely({
       clientEngine: 'node-sdk',
       projectConfigManager,
       errorHandler,
       jsonSchemaValidator,
       logger,
       notificationCenter,
       eventProcessor,
+      odpManager,
       disposable: true,
       isValidInstance: true,
     });
 
     expect(projectConfigManager.makeDisposable).toHaveBeenCalled();
-    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-    // @ts-ignore
-    expect(instance.getProjectConfig()).toBe(projectConfigManager.config);
-    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-    // @ts-ignore
-    expect(projectConfigManager.disposable).toBe(true);
+    expect(eventProcessor.makeDisposable).toHaveBeenCalled();
+    expect(odpManager.makeDisposable).toHaveBeenCalled();
   });
 });

lib/project_config/polling_datafile_manager.spec.ts

@@ -265,6 +265,30 @@ describe('PollingDatafileManager', () => {
       await expect(manager.onTerminated()).rejects.toThrow();
     });
 
+    it('retries min(initRetry, 5) amount of times if datafile manager is disposable', async () => {
+      const repeater = getMockRepeater();
+      const requestHandler = getMockRequestHandler();
+      const mockResponse = getMockAbortableRequest(Promise.reject('test error'));
+      requestHandler.makeRequest.mockReturnValue(mockResponse);
+      
+      const manager = new PollingDatafileManager({
+        repeater,
+        requestHandler,
+        sdkKey: 'keyThatExists',
+        initRetry: 10,
+      });
+      manager.makeDisposable();
+      manager.start();
+
+      for(let i = 0; i < 6; i++) {
+        const ret = repeater.execute(i);
+        await expect(ret).rejects.toThrow();
+      }
+
+      expect(repeater.isRunning()).toBe(false);
+      expect(() => repeater.execute(6)).toThrow();
+    })
+
     it('retries specified number of times before rejecting onRunning() and onTerminated() when autoupdate is false', async () => {
       const repeater = getMockRepeater();
       const requestHandler = getMockRequestHandler();

lib/project_config/polling_datafile_manager.ts

@@ -89,17 +89,18 @@ export class PollingDatafileManager extends BaseService implements DatafileManag
       return;
     }
 
-    if(this.disposable) {
-      this.initRetryRemaining = Math.min(this.initRetryRemaining ?? 5, 5);
-    }
-
     super.start();
     this.state = ServiceState.Starting;
     this.setDatafileFromCacheIfAvailable();
     this.repeater.setTask(this.syncDatafile.bind(this));
     this.repeater.start(true);
   }
 
+  makeDisposable(): void {
+    super.makeDisposable();
+    this.initRetryRemaining = Math.min(this.initRetryRemaining ?? 5, 5);
+  }
+
   stop(): void {
     if (this.isDone()) {
       return;

lib/project_config/project_config_manager.spec.ts

@@ -517,6 +517,15 @@ describe('ProjectConfigManagerImpl', () => {
 
         expect(listener).toHaveBeenCalledTimes(1);
       });
+
+      it('should make datafileManager disposable if makeDisposable() is called', async () => {
+        const datafileManager = getMockDatafileManager({});
+        vi.spyOn(datafileManager, 'makeDisposable');
+        const manager = new ProjectConfigManagerImpl({ datafileManager });
+        manager.makeDisposable();
+
+        expect(datafileManager.makeDisposable).toHaveBeenCalled();
+      })
     });
   });
 });