From 9d060be48ca5a9db50e9a62d77469f94256161ae Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 3 Jul 2024 16:06:55 +0200 Subject: [PATCH] [PM-9442] Add tests for undefined state values and proper emulation of ElectronStorageService in tests (#9910) * fix: handle undefined value in migration 66 * fix: the if-statement was typo * feat: duplicate error behavior in fake storage service * feat: fix all migrations that were setting undefined values * feat: add test for disabled fingrint in migration 66 * fix: default single user state saving undefined value to state * revert: awaiting floating promise gonna fix this in a separate PR * Revert "feat: fix all migrations that were setting undefined values" This reverts commit 034713256cee9a8e164295c88157fe33d8372c81. * feat: automatically convert save to remove * Revert "fix: default single user state saving undefined value to state" This reverts commit 6c36da6ba52f6886d0de2b502b3aaff7f122c3a7. --- .../services/electron-storage.service.ts | 5 +++ libs/common/spec/fake-storage.service.ts | 41 ++++++++++++++++++- .../66-move-final-desktop-settings.spec.ts | 39 ++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/platform/services/electron-storage.service.ts b/apps/desktop/src/platform/services/electron-storage.service.ts index dc145fc2f7..3fa9b2220c 100644 --- a/apps/desktop/src/platform/services/electron-storage.service.ts +++ b/apps/desktop/src/platform/services/electron-storage.service.ts @@ -82,9 +82,14 @@ export class ElectronStorageService implements AbstractStorageService { } save(key: string, obj: unknown): Promise { + if (obj === undefined) { + return this.remove(key); + } + if (obj instanceof Set) { obj = Array.from(obj); } + this.store.set(key, obj); this.updatesSubject.next({ key, updateType: "save" }); return Promise.resolve(); diff --git a/libs/common/spec/fake-storage.service.ts b/libs/common/spec/fake-storage.service.ts index f4817ff581..c6d989c5ab 100644 --- a/libs/common/spec/fake-storage.service.ts +++ b/libs/common/spec/fake-storage.service.ts @@ -8,6 +8,8 @@ import { } from "../src/platform/abstractions/storage.service"; import { StorageOptions } from "../src/platform/models/domain/storage-options"; +const INTERNAL_KEY = "__internal__"; + export class FakeStorageService implements AbstractStorageService, ObservableStorageService { private store: Record; private updatesSubject = new Subject(); @@ -63,13 +65,32 @@ export class FakeStorageService implements AbstractStorageService, ObservableSto this.mock.has(key, options); return Promise.resolve(this.store[key] != null); } - save(key: string, obj: T, options?: StorageOptions): Promise { + async save(key: string, obj: T, options?: StorageOptions): Promise { + // These exceptions are copied from https://github.com/sindresorhus/conf/blob/608adb0c46fb1680ddbd9833043478367a64c120/source/index.ts#L193-L203 + // which is a library that is used by `ElectronStorageService`. We add them here to ensure that the behavior in our testing mirrors the real world. + if (typeof key !== "string" && typeof key !== "object") { + throw new TypeError( + `Expected \`key\` to be of type \`string\` or \`object\`, got ${typeof key}`, + ); + } + + // We don't throw this error because ElectronStorageService automatically detects this case + // and calls `delete()` instead of `set()`. + // if (typeof key !== "object" && obj === undefined) { + // throw new TypeError("Use `delete()` to clear values"); + // } + + if (this._containsReservedKey(key)) { + throw new TypeError( + `Please don't use the ${INTERNAL_KEY} key, as it's used to manage this module internal operations.`, + ); + } + // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. // eslint-disable-next-line @typescript-eslint/no-floating-promises this.mock.save(key, obj, options); this.store[key] = obj; this.updatesSubject.next({ key: key, updateType: "save" }); - return Promise.resolve(); } remove(key: string, options?: StorageOptions): Promise { // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. @@ -79,4 +100,20 @@ export class FakeStorageService implements AbstractStorageService, ObservableSto this.updatesSubject.next({ key: key, updateType: "remove" }); return Promise.resolve(); } + + private _containsReservedKey(key: string | Partial): boolean { + if (typeof key === "object") { + const firsKey = Object.keys(key)[0]; + + if (firsKey === INTERNAL_KEY) { + return true; + } + } + + if (typeof key !== "string") { + return false; + } + + return false; + } } diff --git a/libs/common/src/state-migrations/migrations/66-move-final-desktop-settings.spec.ts b/libs/common/src/state-migrations/migrations/66-move-final-desktop-settings.spec.ts index 19be45640f..5140260f72 100644 --- a/libs/common/src/state-migrations/migrations/66-move-final-desktop-settings.spec.ts +++ b/libs/common/src/state-migrations/migrations/66-move-final-desktop-settings.spec.ts @@ -92,6 +92,45 @@ describe("MoveDesktopSettings", () => { global_desktopSettings_browserIntegrationFingerprintEnabled: false, }, }, + { + it: "migrates browser integration without fingerprint enabled", + preMigration: { + global_account_accounts: { + user1: {}, + otherUser: {}, + }, + user1: { + settings: { + minimizeOnCopyToClipboard: false, + }, + }, + otherUser: { + settings: { + random: "stuff", + }, + }, + global: { + enableBrowserIntegration: true, + }, + }, + postMigration: { + global_account_accounts: { + user1: {}, + otherUser: {}, + }, + global: {}, + user1: { + settings: {}, + }, + otherUser: { + settings: { + random: "stuff", + }, + }, + global_desktopSettings_browserIntegrationEnabled: true, + user_user1_desktopSettings_minimizeOnCopy: false, + }, + }, { it: "does not move non-existant values", preMigration: {