From bea3b8e70ad56325b319beb652488e99bc34e964 Mon Sep 17 00:00:00 2001 From: Carlos Santos <4a.santos@gmail.com> Date: Fri, 14 Nov 2025 12:02:42 +0100 Subject: [PATCH] ov-components: Refactor ActionService and related tests to improve dialog handling and mock implementations --- .../session/session.component.spec.ts | 2 +- .../services/action/action.service.mock.ts | 18 --- .../services/action/action.service.spec.ts | 137 ++++++++++++------ .../src/lib/services/action/action.service.ts | 95 ++++++------ .../src/test-helpers/action.service.mock.ts | 51 +++++++ .../test-helpers/translate.service.mock.ts | 10 ++ 6 files changed, 197 insertions(+), 116 deletions(-) delete mode 100644 openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.mock.ts create mode 100644 openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/action.service.mock.ts create mode 100644 openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/translate.service.mock.ts diff --git a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/components/session/session.component.spec.ts b/openvidu-components-angular/projects/openvidu-components-angular/src/lib/components/session/session.component.spec.ts index bad2ad0f5..60781fa6b 100644 --- a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/components/session/session.component.spec.ts +++ b/openvidu-components-angular/projects/openvidu-components-angular/src/lib/components/session/session.component.spec.ts @@ -3,7 +3,7 @@ import { MatProgressSpinnerModule } from '@angular/material/progress-spinner'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; import { ActionService } from '../../services/action/action.service'; -import { ActionServiceMock } from '../../services/action/action.service.mock'; +import { ActionServiceMock } from '../../../test-helpers/action.service.mock'; import { ChatService } from '../../services/chat/chat.service'; import { ChatServiceMock } from '../../services/chat/chat.service.mock'; diff --git a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.mock.ts b/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.mock.ts deleted file mode 100644 index 25ff82e95..000000000 --- a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.mock.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { Injectable } from '@angular/core'; -import { INotificationOptions } from '../../models/notification-options.model'; - -@Injectable() -export class ActionServiceMock { - constructor() {} - - launchNotification(options: INotificationOptions, callback): void { - - } - - openConnectionDialog(titleMessage: string, descriptionMessage: string, allowClose = true) { - - } - - closeConnectionDialog() { - } -} diff --git a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.spec.ts b/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.spec.ts index 5f9d0447c..746b27522 100644 --- a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.spec.ts +++ b/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.spec.ts @@ -1,81 +1,128 @@ import { fakeAsync, TestBed, tick } from '@angular/core/testing'; -import { MatDialog, MatDialogRef } from '@angular/material/dialog'; +import { of } from 'rxjs'; +import { delay } from 'rxjs/operators'; +import { MatDialog } from '@angular/material/dialog'; import { MatSnackBarModule } from '@angular/material/snack-bar'; - import { ActionService } from './action.service'; -import { TranslateService } from '../translate/translate.service'; -import { TranslateServiceMock } from '../translate/translate.service.mock'; +import { MatSnackBar } from '@angular/material/snack-bar'; +import { MatDialogMock } from '../../../test-helpers/action.service.mock'; +import { TranslateServiceMock } from '../../../test-helpers/translate.service.mock'; -export class MatDialogMock { - open() { - return { close: () => {} } as MatDialogRef; - } -} - -describe('ActionService', () => { +describe('ActionService (characterization)', () => { let service: ActionService; - let dialog: MatDialog; + let dialog: MatDialogMock; beforeEach(() => { TestBed.configureTestingModule({ imports: [MatSnackBarModule], providers: [ { provide: MatDialog, useClass: MatDialogMock }, - { provide: TranslateService, useClass: TranslateServiceMock }, + { provide: 'TranslateService', useClass: TranslateServiceMock }, { provide: 'OPENVIDU_COMPONENTS_CONFIG', useValue: { production: false } } ] }); service = TestBed.inject(ActionService); - dialog = TestBed.inject(MatDialog); + dialog = TestBed.inject(MatDialog) as unknown as MatDialogMock; }); - it('should be created', () => { - expect(service).toBeTruthy(); + it('opens a connection dialog when requested', () => { + const spy = spyOn(dialog, 'open').and.callThrough(); + + service.openConnectionDialog('Title', 'Description', false); + + expect(spy).toHaveBeenCalledTimes(1); + // observable behavior: a MatDialogRef was created (do not assert internal state) + expect(dialog.lastRef).toBeTruthy(); + expect(typeof dialog.lastRef!.close).toBe('function'); }); - it('should open connection dialog', fakeAsync(() => { - const dialogSpy = spyOn(dialog, 'open').and.callThrough(); - service.openConnectionDialog('Test Title', 'Test Description', false); - expect(dialogSpy).toHaveBeenCalled(); - expect(service['isConnectionDialogOpen']).toBeTrue(); - })); + it('does not open a new dialog if one is already open (repeated calls)', () => { + const spy = spyOn(dialog, 'open').and.callThrough(); - it('should not open connection dialog if one is already open', () => { - service['isConnectionDialogOpen'] = true; - const dialogSpy = spyOn(dialog, 'open').and.callThrough(); - service.openConnectionDialog('Test Title', 'Test Description', false); + service.openConnectionDialog('Title', 'Description', false); + // repeated calls simulate concurrent/repeated user attempts + service.openConnectionDialog('Title', 'Description', false); + service.openConnectionDialog('Title', 'Description', false); - expect(dialogSpy).not.toHaveBeenCalled(); + // observed behavior: open called only once + expect(spy).toHaveBeenCalledTimes(1); }); - it('should close connection dialog and reset state', fakeAsync(() => { - service.openConnectionDialog('Test Title', 'Test Description', false); + it('closes the opened dialog when requested and allows opening a new one afterwards', fakeAsync(() => { + const openSpy = spyOn(dialog, 'open').and.callThrough(); - tick(2000); + service.openConnectionDialog('T', 'D', false); + tick(10); // advance microtasks if the service uses timers/async internally - expect(service['isConnectionDialogOpen']).toBeTrue(); + // Behavior: closing should invoke close() on the MatDialogRef + const ref = dialog.lastRef!; + expect(ref).toBeTruthy(); + expect(ref.close).not.toHaveBeenCalled(); service.closeConnectionDialog(); + expect(ref.close).toHaveBeenCalledTimes(1); - expect(service['isConnectionDialogOpen']).toBeFalse(); + // After closing, opening again should create another instance (another open call) + service.openConnectionDialog('T', 'D', false); + expect(openSpy).toHaveBeenCalledTimes(2); })); - it('should open connection dialog only once', fakeAsync(() => { - // Spy on the dialog open method - const dialogSpy = spyOn(dialog, 'open').and.callThrough(); + it('handles rapid consecutive calls by creating a single dialog (reentrancy protection)', fakeAsync(() => { + const spy = spyOn(dialog, 'open').and.callThrough(); - service.openConnectionDialog('Test Title', 'Test Description', false); - // Verify that the dialog has been called only once - expect(dialogSpy).toHaveBeenCalledTimes(1); - expect(service['isConnectionDialogOpen']).toBeTrue(); + // several almost-simultaneous calls + service.openConnectionDialog('T', 'D', false); + service.openConnectionDialog('T', 'D', false); + tick(0); + service.openConnectionDialog('T', 'D', false); + tick(0); - // Try to open the dialog again - service.openConnectionDialog('Test Title', 'Test Description', false); - service.openConnectionDialog('Test Title', 'Test Description', false); - service.openConnectionDialog('Test Title', 'Test Description', false); + expect(spy).toHaveBeenCalledTimes(1); + })); - // Verify that the dialog has been called only once - expect(dialogSpy).toHaveBeenCalledTimes(1); + it('launchNotification uses snackbar and triggers callback on action', fakeAsync(() => { + const snackBar = TestBed.inject( + (window as any).ng && (window as any).ng.material + ? (window as any).ng.material.MatSnackBar + : (require('@angular/material/snack-bar') as any).MatSnackBar + ) as any; + // Fallback: inject via TestBed + const snack = TestBed.inject(MatSnackBar); + const openSpy = spyOn(snack, 'open').and.returnValue({ onAction: () => of(null).pipe(delay(0)) } as any); + + const callback = jasmine.createSpy('callback'); + service.launchNotification({ message: 'hello', buttonActionText: 'OK' }, callback); + // allow the deferred observable to emit + tick(); + + expect(openSpy).toHaveBeenCalled(); + expect(callback).toHaveBeenCalled(); + })); + + it('openDeleteRecordingDialog calls success callback when dialog closes with true', fakeAsync(() => { + const success = jasmine.createSpy('success'); + service.openDeleteRecordingDialog(success); + // MatDialogRefMock.afterClosed returns of(true) so the subscription should call the callback + tick(); + expect(success).toHaveBeenCalledTimes(1); + })); + + it('openRecordingPlayerDialog triggers error handler when dialog returns manageError', fakeAsync(() => { + // Arrange: make dialog.open return a ref that afterClosed emits an object with manageError:true + const returnRef = { + afterClosed: () => ({ subscribe: (fn: any) => fn({ manageError: true, error: { code: 1 } }) }), + close: jasmine.createSpy('close') + } as any; + const openSpy = spyOn(dialog, 'open').and.returnValue(returnRef); + const handleSpy = spyOn(service as any, 'handleRecordingPlayerError').and.callThrough(); + + // Act + service.openRecordingPlayerDialog('someSrc', true); + tick(); + + // Assert + expect(openSpy).toHaveBeenCalled(); + expect(handleSpy).toHaveBeenCalled(); })); }); diff --git a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.ts b/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.ts index ed90e049f..2e2d94130 100644 --- a/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.ts +++ b/openvidu-components-angular/projects/openvidu-components-angular/src/lib/services/action/action.service.ts @@ -19,9 +19,8 @@ export class ActionService { private dialogRef: | MatDialogRef | undefined; - private dialogSubscription: Subscription; private connectionDialogRef: MatDialogRef | undefined; - private isConnectionDialogOpen: boolean = false; + private isConnectionDialogOpen = false; constructor( private snackBar: MatSnackBar, @@ -29,7 +28,7 @@ export class ActionService { private translateService: TranslateService ) {} - launchNotification(options: INotificationOptions, callback): void { + launchNotification(options: INotificationOptions, callback?: () => void): void { if (!options.config) { options.config = { duration: 3000, @@ -41,28 +40,23 @@ export class ActionService { const notification = this.snackBar.open(options.message, options.buttonActionText, options.config); if (callback) { - notification.onAction().subscribe(() => { + // subscribe and complete immediately after calling callback + const sub = notification.onAction().subscribe(() => { + sub.unsubscribe(); callback(); }); } } openDialog(titleMessage: string, descriptionMessage: string, allowClose = true) { - try { - this.closeDialog(); - } catch (error) { - } finally { - const config: MatDialogConfig = { - minWidth: '250px', - data: { title: titleMessage, description: descriptionMessage, showActionButtons: allowClose }, - disableClose: !allowClose - }; - this.dialogRef = this.dialog.open(DialogTemplateComponent, config); - this.dialogSubscription = this.dialogRef.afterClosed().subscribe((result) => { - this.dialogRef = undefined; - if (this.dialogSubscription) this.dialogSubscription.unsubscribe(); - }); - } + this.closeDialog(); + const config: MatDialogConfig = { + minWidth: '250px', + data: { title: titleMessage, description: descriptionMessage, showActionButtons: allowClose }, + disableClose: !allowClose + }; + this.dialogRef = this.dialog.open(DialogTemplateComponent, config); + this.dialogRef.afterClosed().subscribe(() => (this.dialogRef = undefined)); } openConnectionDialog(titleMessage: string, descriptionMessage: string, allowClose = false) { @@ -75,47 +69,44 @@ export class ActionService { this.connectionDialogRef = this.dialog.open(DialogTemplateComponent, config); this.isConnectionDialogOpen = true; + this.connectionDialogRef.afterClosed().subscribe(() => { + this.isConnectionDialogOpen = false; + this.connectionDialogRef = undefined; + }); } - openDeleteRecordingDialog(succsessCallback) { - try { - this.closeDialog(); - } catch (error) { - } finally { - this.dialogRef = this.dialog.open(DeleteDialogComponent); - - this.dialogSubscription = this.dialogRef.afterClosed().subscribe((result) => { - if (result) { - succsessCallback(); - if (this.dialogSubscription) this.dialogSubscription.unsubscribe(); - } - }); - } + openDeleteRecordingDialog(successCallback: () => void) { + this.closeDialog(); + this.dialogRef = this.dialog.open(DeleteDialogComponent); + this.dialogRef.afterClosed().subscribe((result) => { + if (result) { + successCallback(); + } + this.dialogRef = undefined; + }); } openRecordingPlayerDialog(src: string, allowClose = true) { - try { - this.closeDialog(); - } catch (error) { - } finally { - const config: MatDialogConfig = { - minWidth: '250px', - data: { src, showActionButtons: allowClose }, - disableClose: !allowClose - }; - this.dialogRef = this.dialog.open(RecordingDialogComponent, config); - - this.dialogSubscription = this.dialogRef.afterClosed().subscribe((data: { manageError: boolean; error: MediaError | null }) => { - if (data.manageError) { - this.handleRecordingPlayerError(data.error); - } - if (this.dialogSubscription) this.dialogSubscription.unsubscribe(); - }); - } + this.closeDialog(); + const config: MatDialogConfig = { + minWidth: '250px', + data: { src, showActionButtons: allowClose }, + disableClose: !allowClose + }; + this.dialogRef = this.dialog.open(RecordingDialogComponent, config); + this.dialogRef.afterClosed().subscribe((data: { manageError: boolean; error: MediaError | null }) => { + if (data && data.manageError) { + this.handleRecordingPlayerError(data.error); + } + this.dialogRef = undefined; + }); } closeDialog() { - this.dialogRef?.close(); + if (this.dialogRef) { + this.dialogRef.close(); + this.dialogRef = undefined; + } } closeConnectionDialog() { diff --git a/openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/action.service.mock.ts b/openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/action.service.mock.ts new file mode 100644 index 000000000..1b7e33279 --- /dev/null +++ b/openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/action.service.mock.ts @@ -0,0 +1,51 @@ +import { MatDialogRef } from '@angular/material/dialog'; +import { Subject, of } from 'rxjs'; +export class ActionServiceMock { + openConnectionDialog(title?: string, description?: string, allowClose?: boolean): void {} + closeConnectionDialog(): void {} + openDialog(title?: string, description?: string, allowClose?: boolean): void {} + openDeleteRecordingDialog(callback?: () => void): void { + if (callback) callback(); + } + openRecordingPlayerDialog(src?: string, allowClose?: boolean): void {} + launchNotification(options?: any, callback?: () => void): void { + if (callback) callback(); + } +} + +export class MatDialogRefMock { + private closed$ = new Subject(); + // expose a jasmine spy for close so tests can assert it was called + close = jasmine.createSpy('close').and.callFake(() => { + // when close is called, emit and complete the closed observable + this.closed$.next(true); + this.closed$.complete(); + }); + + afterClosed() { + // return an observable that only emits when close() is called + return this.closed$.asObservable(); + } +} + +export class MatDialogMock { + opens = 0; + lastRef: MatDialogRefMock | null = null; + + open(component?: any) { + this.opens++; + // If the consumer opens the DeleteDialogComponent, return a ref that emits immediately + // (some tests expect afterClosed to already have emitted for confirm/delete dialogs) + if (component && component.name === 'DeleteDialogComponent') { + const immediateRef: any = { + close: jasmine.createSpy('close'), + afterClosed: () => of(true) + }; + this.lastRef = immediateRef as unknown as MatDialogRefMock; + return immediateRef as unknown as MatDialogRef; + } + + this.lastRef = new MatDialogRefMock(); + return this.lastRef as unknown as MatDialogRef; + } +} diff --git a/openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/translate.service.mock.ts b/openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/translate.service.mock.ts new file mode 100644 index 000000000..c355aa066 --- /dev/null +++ b/openvidu-components-angular/projects/openvidu-components-angular/src/test-helpers/translate.service.mock.ts @@ -0,0 +1,10 @@ +import { of } from 'rxjs'; + +export class TranslateServiceMock { + instant(key: string): string { + return key; + } + get(key: string) { + return of(key); + } +}