Added tests for restore database webview controller#21566
Added tests for restore database webview controller#21566laurenastrid1 wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds unit test coverage around RestoreDatabaseWebviewController behavior and reducer handlers, along with small initialization flow changes in the existing restore controller (and an additional restore controller implementation introduced in the backup controller module).
Changes:
- Added a comprehensive unit test suite for
RestoreDatabaseWebviewControllercovering initialization paths, reducers, and helper methods. - Adjusted
RestoreDatabaseWebviewController.initializeDialogdefault initialization ordering and view-model update calls. - Introduced a second
RestoreDatabaseWebviewControllerimplementation insidebackupDatabaseWebviewController.ts(in addition to the existing dedicated restore controller file).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| extensions/mssql/test/unit/restoreDatabaseWebviewController.test.ts | New unit tests for restore controller initialization, reducers, and helpers. |
| extensions/mssql/src/controllers/restoreDatabaseWebviewController.ts | Tweaks to restore controller initialization defaults and update calls. |
| extensions/mssql/src/controllers/backupDatabaseWebviewController.ts | Adds a full restore controller implementation (duplicate location) and supporting imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export class RestoreDatabaseWebviewController extends ObjectManagementWebviewController< | ||
| RestoreDatabaseFormState, | ||
| RestoreDatabaseReducers<RestoreDatabaseFormState> | ||
| > { |
There was a problem hiding this comment.
PR description/title indicate only adding tests, but this change adds a large new production controller (RestoreDatabaseWebviewController) into backupDatabaseWebviewController.ts and also modifies the existing restore controller. Please confirm whether these production code changes are intentional and update the PR description accordingly (or revert the unrelated additions).
| controller = new RestoreDatabaseWebviewController( | ||
| mockContext, | ||
| vscodeWrapper, | ||
| mockObjectManagementService, | ||
| mockConnectionManager, | ||
| mockFileBrowserService, | ||
| mockAzureBlobService, | ||
| mockProfile, | ||
| "ownerUri", | ||
| mockProfile.database, | ||
| ); |
There was a problem hiding this comment.
mockFileBrowserService is declared but never assigned before being passed into the RestoreDatabaseWebviewController constructor. This will trigger TS2454 (used before assignment) and can also cause runtime failures when file browser reducers are registered. Initialize it in setup (e.g., create a stub instance) before constructing the controller.
|
|
||
| getRestoreConfigInfoStub = | ||
| mockObjectManagementService.getRestoreConfigInfo as sinon.SinonStub; | ||
| mockObjectManagementService.getBackupConfigInfo as sinon.SinonStub; |
There was a problem hiding this comment.
This line is a no-op expression (mockObjectManagementService.getBackupConfigInfo as sinon.SinonStub;) and looks like leftover scaffolding. It can be flagged by linting/no-unused-expressions rules; remove it or assign it to a variable if you intended to configure the stub.
| mockObjectManagementService.getBackupConfigInfo as sinon.SinonStub; |
There was a problem hiding this comment.
You could probably remove this since it's not being assigned to anything.
| mockInitialState.formComponents = controller[ | ||
| "setFormComponents" | ||
| ] as typeof mockInitialState.formComponents; |
There was a problem hiding this comment.
mockInitialState.formComponents is being assigned to the setFormComponents function reference instead of the function result. This makes formComponents incorrect for later tests that spread/use it. Call controller["setFormComponents"]() (or reuse controller.state.formComponents) to capture the actual components object.
| mockInitialState.formComponents = controller[ | |
| "setFormComponents" | |
| ] as typeof mockInitialState.formComponents; | |
| mockInitialState.formComponents = controller["state"] | |
| .formComponents as typeof mockInitialState.formComponents; |
| controller = new RestoreDatabaseWebviewController( | ||
| mockContext, | ||
| vscodeWrapper, | ||
| mockObjectManagementService, | ||
| mockConnectionManager, | ||
| mockFileBrowserService, | ||
| mockAzureBlobService, | ||
| mockProfile, | ||
| "ownerUri", | ||
| mockProfile.database, | ||
| ); | ||
|
|
||
| mockInitialState = { | ||
| viewModel: { | ||
| dialogType: ObjectManagementDialogType.RestoreDatabase, | ||
| model: { | ||
| loadState: ApiStatus.Loaded, | ||
| azureComponentStatuses: { | ||
| accountId: ApiStatus.NotStarted, | ||
| tenantId: ApiStatus.NotStarted, | ||
| subscriptionId: ApiStatus.NotStarted, | ||
| storageAccountId: ApiStatus.NotStarted, | ||
| blobContainerId: ApiStatus.NotStarted, | ||
| }, | ||
| type: DisasterRecoveryType.BackupFile, | ||
| backupFiles: [ | ||
| { | ||
| filePath: `${mockConfigInfo.configInfo.defaultBackupFolder}/${defaultBackupName}`, | ||
| isExisting: false, | ||
| }, | ||
| ], | ||
| tenants: [], | ||
| subscriptions: [], | ||
| storageAccounts: [], | ||
| blobContainers: [], | ||
| url: "", | ||
| serverName: "serverName", | ||
| restorePlan: undefined, | ||
| restorePlanStatus: ApiStatus.NotStarted, | ||
| blobs: [], | ||
| cachedRestorePlanParams: undefined, | ||
| selectedBackupSets: [], | ||
| } as RestoreDatabaseViewModel, | ||
| }, | ||
| ownerUri: "ownerUri", | ||
| databaseName: "testDatabase", | ||
| defaultFileBrowserExpandPath: mockConfigInfo.configInfo.defaultBackupFolder, | ||
| formState: { | ||
| sourceDatabaseName: "", | ||
| targetDatabaseName: "", | ||
| accountId: "", | ||
| tenantId: "", | ||
| subscriptionId: "", | ||
| storageAccountId: "", | ||
| blobContainerId: "", | ||
| dataFileFolder: mockConfigInfo.configInfo.dataFileFolder, | ||
| logFileFolder: mockConfigInfo.configInfo.logFileFolder, | ||
| } as RestoreDatabaseFormState, | ||
| formComponents: {}, | ||
| fileBrowserState: undefined, | ||
| dialog: undefined, | ||
| formErrors: [], | ||
| fileFilterOptions: [ | ||
| { | ||
| displayName: LocConstants.BackupDatabase.backupFileTypes, | ||
| value: defaultBackupFileTypes, | ||
| }, | ||
| { | ||
| displayName: LocConstants.BackupDatabase.allFiles, | ||
| value: allFileTypes, | ||
| }, | ||
| ], | ||
| } as ObjectManagementWebviewState<RestoreDatabaseFormState>; | ||
|
|
||
| await controller["initializeDialog"](); | ||
|
|
There was a problem hiding this comment.
The controller constructor calls start(), which kicks off an async initializeDialog() automatically. The test then calls initializeDialog() again, causing multiple overlapping initializations and potential race/flakiness. Consider stubbing RestoreDatabaseWebviewController.prototype.start to a no-op (or using a test subclass) so the test controls when initialization runs.
| expect(result.options["backupFilePaths"]).to.equal(""); | ||
| expect(result.options["readHeaderFromMedia"]).to.be.false; | ||
| expect(result.readHeaderFromMedia).to.be.false; | ||
| // AssertionError: expected '' to equal 'testDatabase' |
There was a problem hiding this comment.
The inline // AssertionError: expected '' to equal 'testDatabase' looks like leftover debugging output. Please remove it (or replace with a brief comment describing the intended behavior) to keep the test readable.
| // AssertionError: expected '' to equal 'testDatabase' | |
| // Expect the source database name to be preserved from the state |
|
|
||
| void this.getRestorePlan(false) | ||
| .then((state) => { | ||
| restoreViewModel = this.setDefaultFormValuesFromPlan(state); |
There was a problem hiding this comment.
setDefaultFormValuesFromPlan(state) mutates state.formState / view model defaults, but the result is no longer propagated back to the webview (the previous updateViewModel(..., state) call was removed). This makes the default values computed from the restore plan effectively invisible to the UI. Consider calling updateState(state) or updating the view model/state after applying defaults so the webview receives the updated values.
| restoreViewModel = this.setDefaultFormValuesFromPlan(state); | |
| restoreViewModel = this.setDefaultFormValuesFromPlan(state); | |
| this.updateViewModel(restoreViewModel); |
| export class RestoreDatabaseWebviewController extends ObjectManagementWebviewController< | ||
| RestoreDatabaseFormState, | ||
| RestoreDatabaseReducers<RestoreDatabaseFormState> | ||
| > { |
There was a problem hiding this comment.
This file now contains a full RestoreDatabaseWebviewController implementation. There is already an exported RestoreDatabaseWebviewController in src/controllers/restoreDatabaseWebviewController.ts; duplicating the controller here will diverge over time and makes it ambiguous which module should be imported. Please remove the restore controller from backupDatabaseWebviewController.ts (and keep it in its dedicated file), or refactor to a shared helper if code reuse is the goal.
| //#endregion | ||
| } | ||
|
|
||
| export class RestoreDatabaseWebviewController extends ObjectManagementWebviewController< |
There was a problem hiding this comment.
What's the difference between the RestoreDatabaseWebViewController class here and the one defined in restoreDatabaseWebviewController.ts? The two classes look identical. You might want to remove this one.
| azureProfile.database, | ||
| ); | ||
|
|
||
| let getRestorePlanStub = sandbox.stub(azureController as any, "getRestorePlan"); |
There was a problem hiding this comment.
Are you able to clean up some of the any usages in this file? Public methods/services could use sandbox.createStubInstance<T>().
If your using any types to mock private methods, then sinon won't be able to help there, but you could try exercising private methods indirectly through public reducers and mocking any injected services with createStubInstance.
| let mockObjectManagementService: ObjectManagementService; | ||
| let mockProfile: ConnectionProfile; | ||
| let mockConnectionManager: sinon.SinonStubbedInstance<ConnectionManager>; | ||
| let mockFileBrowserService: FileBrowserService; |
There was a problem hiding this comment.
You can probably remove this. It's only being passed into the controller constructor and since it's not setup or interacted with as a mock this is just being set to undefined
| let mockInitialState: ObjectManagementWebviewState<RestoreDatabaseFormState>; | ||
| let vscodeWrapper: sinon.SinonStubbedInstance<VscodeWrapper>; | ||
| let getRestoreConfigInfoStub: sinon.SinonStub; | ||
| // let getRestorePlanStub: sinon.SinonStub; |
There was a problem hiding this comment.
Remove this commented out code.
| .stub(happyController as any, "setDefaultFormValuesFromPlan") | ||
| .returns(mockInitialState.viewModel.model); | ||
| await happyController["initializeDialog"](); | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); |
There was a problem hiding this comment.
Replace this line with await Promise.resolve();
| sandbox.restore(); | ||
| }); | ||
|
|
||
| test("should initialize with correct state", async () => { |
There was a problem hiding this comment.
This test has 5 paths. See if you can break each path into it's own unit test.
Description
Added tests for restoreDatabaseWebviewController
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines