From b204d2cf5a2bd66476642433637cbcf4f58e665f Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Thu, 28 Nov 2024 14:57:29 +0800 Subject: [PATCH 1/7] add lock for refresh operation --- src/AzureAppConfigurationImpl.ts | 7 +++++++ src/common/lock.ts | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 src/common/lock.ts diff --git a/src/AzureAppConfigurationImpl.ts b/src/AzureAppConfigurationImpl.ts index 1bbf0779..d035c362 100644 --- a/src/AzureAppConfigurationImpl.ts +++ b/src/AzureAppConfigurationImpl.ts @@ -9,6 +9,7 @@ import { IKeyValueAdapter } from "./IKeyValueAdapter.js"; import { JsonKeyValueAdapter } from "./JsonKeyValueAdapter.js"; import { DEFAULT_REFRESH_INTERVAL_IN_MS, MIN_REFRESH_INTERVAL_IN_MS } from "./RefreshOptions.js"; import { Disposable } from "./common/disposable.js"; +import { Lock } from "./common/lock.js" import { FEATURE_FLAGS_KEY_NAME, FEATURE_MANAGEMENT_KEY_NAME } from "./featureManagement/constants.js"; import { AzureKeyVaultKeyValueAdapter } from "./keyvault/AzureKeyVaultKeyValueAdapter.js"; import { RefreshTimer } from "./refresh/RefreshTimer.js"; @@ -40,6 +41,8 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { #isInitialLoadCompleted: boolean = false; // Refresh + #refreshLock: Lock = new Lock(); // lock to prevent "concurrent" async refresh + #refreshInterval: number = DEFAULT_REFRESH_INTERVAL_IN_MS; #onRefreshListeners: Array<() => any> = []; /** @@ -350,6 +353,10 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { throw new Error("Refresh is not enabled for key-values or feature flags."); } + this.#refreshLock.execute(this.#refreshTasks); + } + + async #refreshTasks(): Promise { const refreshTasks: Promise[] = []; if (this.#refreshEnabled) { refreshTasks.push(this.#refreshKeyValues()); diff --git a/src/common/lock.ts b/src/common/lock.ts new file mode 100644 index 00000000..89883227 --- /dev/null +++ b/src/common/lock.ts @@ -0,0 +1,18 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +export class Lock { + #locked = false; + + async execute(fn) { + if (this.#locked) { + return; // do nothing + } + this.#locked = true; + try { + await fn(); + } finally { + this.#locked = false; + } + } +} \ No newline at end of file From 9b06257b3c09eaf60c18bf72cbc3d0d0c257d891 Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Thu, 28 Nov 2024 15:08:26 +0800 Subject: [PATCH 2/7] bind context --- src/AzureAppConfigurationImpl.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AzureAppConfigurationImpl.ts b/src/AzureAppConfigurationImpl.ts index d035c362..a193601e 100644 --- a/src/AzureAppConfigurationImpl.ts +++ b/src/AzureAppConfigurationImpl.ts @@ -353,7 +353,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { throw new Error("Refresh is not enabled for key-values or feature flags."); } - this.#refreshLock.execute(this.#refreshTasks); + await this.#refreshLock.execute(this.#refreshTasks.bind(this)); } async #refreshTasks(): Promise { From 552524423f1a0ecdbb279a698697214a1e467ada Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Thu, 28 Nov 2024 15:12:53 +0800 Subject: [PATCH 3/7] fix lint --- src/AzureAppConfigurationImpl.ts | 2 +- src/common/lock.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AzureAppConfigurationImpl.ts b/src/AzureAppConfigurationImpl.ts index a193601e..af7c9d5f 100644 --- a/src/AzureAppConfigurationImpl.ts +++ b/src/AzureAppConfigurationImpl.ts @@ -9,7 +9,7 @@ import { IKeyValueAdapter } from "./IKeyValueAdapter.js"; import { JsonKeyValueAdapter } from "./JsonKeyValueAdapter.js"; import { DEFAULT_REFRESH_INTERVAL_IN_MS, MIN_REFRESH_INTERVAL_IN_MS } from "./RefreshOptions.js"; import { Disposable } from "./common/disposable.js"; -import { Lock } from "./common/lock.js" +import { Lock } from "./common/lock.js"; import { FEATURE_FLAGS_KEY_NAME, FEATURE_MANAGEMENT_KEY_NAME } from "./featureManagement/constants.js"; import { AzureKeyVaultKeyValueAdapter } from "./keyvault/AzureKeyVaultKeyValueAdapter.js"; import { RefreshTimer } from "./refresh/RefreshTimer.js"; diff --git a/src/common/lock.ts b/src/common/lock.ts index 89883227..7166e15c 100644 --- a/src/common/lock.ts +++ b/src/common/lock.ts @@ -15,4 +15,4 @@ export class Lock { this.#locked = false; } } -} \ No newline at end of file +} From 1dead40756faa4a01396ae661f8d0743f47d9235 Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Fri, 29 Nov 2024 16:26:48 +0800 Subject: [PATCH 4/7] update --- src/AzureAppConfigurationImpl.ts | 6 +++--- src/common/lock.ts | 18 ------------------ src/common/mutex.ts | 21 +++++++++++++++++++++ 3 files changed, 24 insertions(+), 21 deletions(-) delete mode 100644 src/common/lock.ts create mode 100644 src/common/mutex.ts diff --git a/src/AzureAppConfigurationImpl.ts b/src/AzureAppConfigurationImpl.ts index af7c9d5f..2acf7fe0 100644 --- a/src/AzureAppConfigurationImpl.ts +++ b/src/AzureAppConfigurationImpl.ts @@ -9,7 +9,7 @@ import { IKeyValueAdapter } from "./IKeyValueAdapter.js"; import { JsonKeyValueAdapter } from "./JsonKeyValueAdapter.js"; import { DEFAULT_REFRESH_INTERVAL_IN_MS, MIN_REFRESH_INTERVAL_IN_MS } from "./RefreshOptions.js"; import { Disposable } from "./common/disposable.js"; -import { Lock } from "./common/lock.js"; +import { ExclusiveExecutor } from "./common/mutex.js"; import { FEATURE_FLAGS_KEY_NAME, FEATURE_MANAGEMENT_KEY_NAME } from "./featureManagement/constants.js"; import { AzureKeyVaultKeyValueAdapter } from "./keyvault/AzureKeyVaultKeyValueAdapter.js"; import { RefreshTimer } from "./refresh/RefreshTimer.js"; @@ -41,7 +41,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { #isInitialLoadCompleted: boolean = false; // Refresh - #refreshLock: Lock = new Lock(); // lock to prevent "concurrent" async refresh + #refreshExecutor: ExclusiveExecutor = new ExclusiveExecutor(); #refreshInterval: number = DEFAULT_REFRESH_INTERVAL_IN_MS; #onRefreshListeners: Array<() => any> = []; @@ -353,7 +353,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { throw new Error("Refresh is not enabled for key-values or feature flags."); } - await this.#refreshLock.execute(this.#refreshTasks.bind(this)); + await this.#refreshExecutor.execute(this.#refreshTasks.bind(this)); } async #refreshTasks(): Promise { diff --git a/src/common/lock.ts b/src/common/lock.ts deleted file mode 100644 index 7166e15c..00000000 --- a/src/common/lock.ts +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -export class Lock { - #locked = false; - - async execute(fn) { - if (this.#locked) { - return; // do nothing - } - this.#locked = true; - try { - await fn(); - } finally { - this.#locked = false; - } - } -} diff --git a/src/common/mutex.ts b/src/common/mutex.ts new file mode 100644 index 00000000..64a4afcc --- /dev/null +++ b/src/common/mutex.ts @@ -0,0 +1,21 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +export class ExclusiveExecutor { + isExecuting = false; + + /** + * Execute the given function exclusively. If there is any previous execution in progress, the new execution will be aborted. + */ + async execute(fn) { + if (this.isExecuting) { + return; // do nothing + } + this.isExecuting = true; + try { + await fn(); + } finally { + this.isExecuting = false; + } + } +} From e17e2956d260bb6f966272ce5abc60355afb69b2 Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Fri, 29 Nov 2024 17:02:09 +0800 Subject: [PATCH 5/7] simplify --- src/AzureAppConfigurationImpl.ts | 13 ++++++++++--- src/common/mutex.ts | 21 --------------------- 2 files changed, 10 insertions(+), 24 deletions(-) delete mode 100644 src/common/mutex.ts diff --git a/src/AzureAppConfigurationImpl.ts b/src/AzureAppConfigurationImpl.ts index 2acf7fe0..1541c40b 100644 --- a/src/AzureAppConfigurationImpl.ts +++ b/src/AzureAppConfigurationImpl.ts @@ -9,7 +9,6 @@ import { IKeyValueAdapter } from "./IKeyValueAdapter.js"; import { JsonKeyValueAdapter } from "./JsonKeyValueAdapter.js"; import { DEFAULT_REFRESH_INTERVAL_IN_MS, MIN_REFRESH_INTERVAL_IN_MS } from "./RefreshOptions.js"; import { Disposable } from "./common/disposable.js"; -import { ExclusiveExecutor } from "./common/mutex.js"; import { FEATURE_FLAGS_KEY_NAME, FEATURE_MANAGEMENT_KEY_NAME } from "./featureManagement/constants.js"; import { AzureKeyVaultKeyValueAdapter } from "./keyvault/AzureKeyVaultKeyValueAdapter.js"; import { RefreshTimer } from "./refresh/RefreshTimer.js"; @@ -41,7 +40,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { #isInitialLoadCompleted: boolean = false; // Refresh - #refreshExecutor: ExclusiveExecutor = new ExclusiveExecutor(); + #refreshInProgress: boolean = false; #refreshInterval: number = DEFAULT_REFRESH_INTERVAL_IN_MS; #onRefreshListeners: Array<() => any> = []; @@ -353,7 +352,15 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { throw new Error("Refresh is not enabled for key-values or feature flags."); } - await this.#refreshExecutor.execute(this.#refreshTasks.bind(this)); + if (this.#refreshInProgress) { + return; + } + this.#refreshInProgress = true; + try { + await this.#refreshTasks(); + } finally { + this.#refreshInProgress = false; + } } async #refreshTasks(): Promise { diff --git a/src/common/mutex.ts b/src/common/mutex.ts deleted file mode 100644 index 64a4afcc..00000000 --- a/src/common/mutex.ts +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -export class ExclusiveExecutor { - isExecuting = false; - - /** - * Execute the given function exclusively. If there is any previous execution in progress, the new execution will be aborted. - */ - async execute(fn) { - if (this.isExecuting) { - return; // do nothing - } - this.isExecuting = true; - try { - await fn(); - } finally { - this.isExecuting = false; - } - } -} From cfb838a4f25665eb9732bd2d80a05dd9c94a023f Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Mon, 2 Dec 2024 16:46:43 +0800 Subject: [PATCH 6/7] add more testcases --- package.json | 2 +- test/featureFlag.test.ts | 2 +- test/json.test.ts | 8 +-- test/keyvault.test.ts | 2 +- test/load.test.ts | 2 +- test/refresh.test.ts | 98 +++++++++++++++++++++++++++++++++---- test/requestTracing.test.ts | 4 +- test/utils/testHelper.ts | 15 ++++-- 8 files changed, 109 insertions(+), 24 deletions(-) diff --git a/package.json b/package.json index 0b493ea3..b194a724 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "dev": "rollup --config --watch", "lint": "eslint src/ test/", "fix-lint": "eslint src/ test/ --fix", - "test": "mocha out/test/*.test.{js,cjs,mjs} --parallel" + "test": "mocha out/test/refresh.test.{js,cjs,mjs} --parallel" }, "repository": { "type": "git", diff --git a/test/featureFlag.test.ts b/test/featureFlag.test.ts index 1bf8eae2..897efe97 100644 --- a/test/featureFlag.test.ts +++ b/test/featureFlag.test.ts @@ -60,7 +60,7 @@ describe("feature flags", function () { this.timeout(10000); before(() => { - mockAppConfigurationClientListConfigurationSettings(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs]); }); after(() => { diff --git a/test/json.test.ts b/test/json.test.ts index c139d53b..47a3f670 100644 --- a/test/json.test.ts +++ b/test/json.test.ts @@ -20,7 +20,7 @@ describe("json", function () { }); it("should load and parse if content type is application/json", async () => { - mockAppConfigurationClientListConfigurationSettings([jsonKeyValue]); + mockAppConfigurationClientListConfigurationSettings([[jsonKeyValue]]); const connectionString = createMockedConnectionString(); const settings = await load(connectionString); @@ -34,7 +34,7 @@ describe("json", function () { }); it("should not parse key-vault reference", async () => { - mockAppConfigurationClientListConfigurationSettings([jsonKeyValue, keyVaultKeyValue]); + mockAppConfigurationClientListConfigurationSettings([[jsonKeyValue, keyVaultKeyValue]]); const connectionString = createMockedConnectionString(); const settings = await load(connectionString, { @@ -50,7 +50,7 @@ describe("json", function () { }); it("should parse different kinds of legal values", async () => { - mockAppConfigurationClientListConfigurationSettings([ + mockAppConfigurationClientListConfigurationSettings([[ /** * A JSON value MUST be an object, array, number, or string, false, null, true * See https://www.ietf.org/rfc/rfc4627.txt @@ -69,7 +69,7 @@ describe("json", function () { createMockedJsonKeyValue("json.settings.emptyString", ""), // should fail JSON.parse and use string value as fallback createMockedJsonKeyValue("json.settings.illegalString", "[unclosed"), // should fail JSON.parse - ]); + ]]); const connectionString = createMockedConnectionString(); const settings = await load(connectionString); expect(settings).not.undefined; diff --git a/test/keyvault.test.ts b/test/keyvault.test.ts index cf01235c..2877243b 100644 --- a/test/keyvault.test.ts +++ b/test/keyvault.test.ts @@ -19,7 +19,7 @@ const mockedData = [ function mockAppConfigurationClient() { // eslint-disable-next-line @typescript-eslint/no-unused-vars const kvs = mockedData.map(([key, vaultUri, _value]) => createMockedKeyVaultReference(key, vaultUri)); - mockAppConfigurationClientListConfigurationSettings(kvs); + mockAppConfigurationClientListConfigurationSettings([kvs]); } function mockNewlyCreatedKeyVaultSecretClients() { diff --git a/test/load.test.ts b/test/load.test.ts index ce22b1a8..6d2c94b8 100644 --- a/test/load.test.ts +++ b/test/load.test.ts @@ -80,7 +80,7 @@ describe("load", function () { this.timeout(10000); before(() => { - mockAppConfigurationClientListConfigurationSettings(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs]); }); after(() => { diff --git a/test/refresh.test.ts b/test/refresh.test.ts index fcffaee5..9749ace8 100644 --- a/test/refresh.test.ts +++ b/test/refresh.test.ts @@ -23,6 +23,15 @@ function addSetting(key: string, value: any) { mockedKVs.push(createMockedKeyValue({ key, value })); } +let listKvRequestCount = 0; +const listKvCallback = () => { + listKvRequestCount++; +}; +let getKvRequestCount = 0; +const getKvCallback = () => { + getKvRequestCount++; +}; + describe("dynamic refresh", function () { this.timeout(10000); @@ -32,12 +41,14 @@ describe("dynamic refresh", function () { { value: "40", key: "app.settings.fontSize" }, { value: "30", key: "app.settings.fontSize", label: "prod" } ].map(createMockedKeyValue); - mockAppConfigurationClientListConfigurationSettings(mockedKVs); - mockAppConfigurationClientGetConfigurationSetting(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting(mockedKVs, getKvCallback); }); afterEach(() => { restoreMocks(); + listKvRequestCount = 0; + getKvRequestCount = 0; }); it("should throw error when refresh is not enabled but refresh is called", async () => { @@ -139,6 +150,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -149,10 +162,14 @@ describe("dynamic refresh", function () { // within refreshInterval, should not really refresh await settings.refresh(); expect(settings.get("app.settings.fontColor")).eq("red"); + expect(listKvRequestCount).eq(1); // no more request should be sent during the refresh interval + expect(getKvRequestCount).eq(0); // no more request should be sent during the refresh interval // after refreshInterval, should really refresh await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(1); expect(settings.get("app.settings.fontColor")).eq("blue"); }); @@ -167,6 +184,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -174,11 +193,13 @@ describe("dynamic refresh", function () { // delete setting 'app.settings.fontColor' const newMockedKVs = mockedKVs.filter(elem => elem.key !== "app.settings.fontColor"); restoreMocks(); - mockAppConfigurationClientListConfigurationSettings(newMockedKVs); - mockAppConfigurationClientGetConfigurationSetting(newMockedKVs); + mockAppConfigurationClientListConfigurationSettings([newMockedKVs], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting(newMockedKVs, getKvCallback); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(2); // one conditional request to detect change and one request as part of loading all kvs (because app.settings.fontColor doesn't exist in the response of listKv request) expect(settings.get("app.settings.fontColor")).eq(undefined); }); @@ -193,6 +214,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -200,6 +223,8 @@ describe("dynamic refresh", function () { updateSetting("app.settings.fontSize", "50"); // unwatched setting await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(1); expect(settings.get("app.settings.fontSize")).eq("40"); }); @@ -215,6 +240,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -224,6 +251,8 @@ describe("dynamic refresh", function () { updateSetting("app.settings.fontSize", "50"); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(2); // two getKv request for two watched settings expect(settings.get("app.settings.fontSize")).eq("50"); expect(settings.get("app.settings.bgColor")).eq("white"); }); @@ -309,6 +338,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(1); // app.settings.bgColor doesn't exist in the response of listKv request, so an additional getKv request is made to get it. expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -317,10 +348,45 @@ describe("dynamic refresh", function () { updateSetting("app.settings.fontColor", "blue"); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(2); // should not refresh expect(settings.get("app.settings.fontColor")).eq("red"); }); + it("should not refresh any more when there is refresh in progress", async () => { + const connectionString = createMockedConnectionString(); + const settings = await load(connectionString, { + refreshOptions: { + enabled: true, + refreshIntervalInMs: 2000, + watchedSettings: [ + { key: "app.settings.fontColor" } + ] + } + }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); + expect(settings).not.undefined; + expect(settings.get("app.settings.fontColor")).eq("red"); + + // change setting + updateSetting("app.settings.fontColor", "blue"); + + // after refreshInterval, should really refresh + await sleepInMs(2 * 1000 + 1); + for (let i = 0; i < 5; i++) { + settings.refresh(); // refresh "concurrently" + } + expect(listKvRequestCount).to.be.at.most(2); + expect(getKvRequestCount).to.be.at.most(1); + + await sleepInMs(1000); + + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(1); + expect(settings.get("app.settings.fontColor")).eq("blue"); + }); }); describe("dynamic refresh feature flags", function () { @@ -331,14 +397,16 @@ describe("dynamic refresh feature flags", function () { afterEach(() => { restoreMocks(); + listKvRequestCount = 0; + getKvRequestCount = 0; }); it("should refresh feature flags when enabled", async () => { mockedKVs = [ createMockedFeatureFlag("Beta", { enabled: true }) ]; - mockAppConfigurationClientListConfigurationSettings(mockedKVs); - mockAppConfigurationClientGetConfigurationSetting(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting(mockedKVs, getKvCallback); const connectionString = createMockedConnectionString(); const settings = await load(connectionString, { @@ -353,6 +421,8 @@ describe("dynamic refresh feature flags", function () { } } }); + expect(listKvRequestCount).eq(2); // one listKv request for kvs and one listKv request for feature flags + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("feature_management")).not.undefined; expect(settings.get("feature_management").feature_flags).not.undefined; @@ -371,6 +441,8 @@ describe("dynamic refresh feature flags", function () { await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(4); // 2 + 2 more requests: one conditional request to detect change and one request to reload all feature flags + expect(getKvRequestCount).eq(0); expect(settings.get("feature_management").feature_flags[0].id).eq("Beta"); expect(settings.get("feature_management").feature_flags[0].enabled).eq(false); @@ -387,8 +459,8 @@ describe("dynamic refresh feature flags", function () { createMockedFeatureFlag("Beta_1", { enabled: true }), createMockedFeatureFlag("Beta_2", { enabled: true }), ]; - mockAppConfigurationClientListConfigurationSettings(page1, page2); - mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2]); + mockAppConfigurationClientListConfigurationSettings([page1, page2], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2], getKvCallback); const connectionString = createMockedConnectionString(); const settings = await load(connectionString, { @@ -403,6 +475,8 @@ describe("dynamic refresh feature flags", function () { } } }); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(0); let refreshSuccessfulCount = 0; settings.onRefresh(() => { @@ -411,16 +485,20 @@ describe("dynamic refresh feature flags", function () { await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(3); // one conditional request to detect change + expect(getKvRequestCount).eq(0); expect(refreshSuccessfulCount).eq(0); // no change in feature flags, because page etags are the same. // change feature flag Beta_1 to false page2[0] = createMockedFeatureFlag("Beta_1", { enabled: false }); restoreMocks(); - mockAppConfigurationClientListConfigurationSettings(page1, page2); - mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2]); + mockAppConfigurationClientListConfigurationSettings([page1, page2], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2], getKvCallback); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(5); // 3 + 2 more requests: one conditional request to detect change and one request to reload all feature flags + expect(getKvRequestCount).eq(0); expect(refreshSuccessfulCount).eq(1); // change in feature flags, because page etags are different. }); }); diff --git a/test/requestTracing.test.ts b/test/requestTracing.test.ts index d4e7edcf..a64d2c4a 100644 --- a/test/requestTracing.test.ts +++ b/test/requestTracing.test.ts @@ -123,10 +123,10 @@ describe("request tracing", function () { }); it("should have request type in correlation-context header when refresh is enabled", async () => { - mockAppConfigurationClientListConfigurationSettings([{ + mockAppConfigurationClientListConfigurationSettings([[{ key: "app.settings.fontColor", value: "red" - }].map(createMockedKeyValue)); + }].map(createMockedKeyValue)]); const settings = await load(createMockedConnectionString(fakeEndpoint), { clientOptions, diff --git a/test/utils/testHelper.ts b/test/utils/testHelper.ts index 6e787dd7..bf05882d 100644 --- a/test/utils/testHelper.ts +++ b/test/utils/testHelper.ts @@ -42,13 +42,16 @@ function _filterKVs(unfilteredKvs: ConfigurationSetting[], listOptions: any) { * Mocks the listConfigurationSettings method of AppConfigurationClient to return the provided pages of ConfigurationSetting. * E.g. * - mockAppConfigurationClientListConfigurationSettings([item1, item2, item3]) // single page - * - mockAppConfigurationClientListConfigurationSettings([item1, item2], [item3], [item4]) // multiple pages * * @param pages List of pages, each page is a list of ConfigurationSetting */ -function mockAppConfigurationClientListConfigurationSettings(...pages: ConfigurationSetting[][]) { +function mockAppConfigurationClientListConfigurationSettings(pages: ConfigurationSetting[][], customCallback?: (listOptions) => any) { sinon.stub(AppConfigurationClient.prototype, "listConfigurationSettings").callsFake((listOptions) => { + if (customCallback) { + customCallback(listOptions); + } + let kvs = _filterKVs(pages.flat(), listOptions); const mockIterator: AsyncIterableIterator & { byPage(): AsyncIterableIterator } = { [Symbol.asyncIterator](): AsyncIterableIterator { @@ -94,8 +97,12 @@ function mockAppConfigurationClientListConfigurationSettings(...pages: Configura }); } -function mockAppConfigurationClientGetConfigurationSetting(kvList) { +function mockAppConfigurationClientGetConfigurationSetting(kvList, customCallback?: (options) => any) { sinon.stub(AppConfigurationClient.prototype, "getConfigurationSetting").callsFake((settingId, options) => { + if (customCallback) { + customCallback(options); + } + const found = kvList.find(elem => elem.key === settingId.key && elem.label === settingId.label); if (found) { if (options?.onlyIfChanged && settingId.etag === found.etag) { @@ -210,4 +217,4 @@ export { createMockedFeatureFlag, sleepInMs -}; +}; From b5759d0573f6b405fa7a3f36dcc9eafd4a3f4d5b Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Mon, 2 Dec 2024 16:50:07 +0800 Subject: [PATCH 7/7] add more comments --- test/refresh.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/refresh.test.ts b/test/refresh.test.ts index 9749ace8..5fbeb973 100644 --- a/test/refresh.test.ts +++ b/test/refresh.test.ts @@ -375,13 +375,13 @@ describe("dynamic refresh", function () { // after refreshInterval, should really refresh await sleepInMs(2 * 1000 + 1); - for (let i = 0; i < 5; i++) { + for (let i = 0; i < 5; i++) { // in practice, refresh should not be used in this way settings.refresh(); // refresh "concurrently" } expect(listKvRequestCount).to.be.at.most(2); expect(getKvRequestCount).to.be.at.most(1); - await sleepInMs(1000); + await sleepInMs(1000); // wait for all 5 refresh attempts to finish expect(listKvRequestCount).eq(2); expect(getKvRequestCount).eq(1);