From 6eb6307940ffd883bb4bae492a2250c599056c38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 30 Jul 2025 14:53:17 +0200 Subject: [PATCH] Fix and replace tests --- .../feature-toggle/feature-toggle-service.ts | 11 +- .../tests/feature-toggle-service.e2e.test.ts | 45 ++++---- ...eature-toggle-strategies-store.e2e.test.ts | 100 +++--------------- .../tests/feature-toggles.e2e.test.ts | 2 - 4 files changed, 46 insertions(+), 112 deletions(-) diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index adfb6e8f7a..864800814d 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -685,13 +685,13 @@ export class FeatureToggleService { delete params?.stickiness; } return { - rollout: '100', + rollout: params?.rollout ?? '100', stickiness: params?.stickiness ?? (await this.featureStrategiesStore.getDefaultStickiness( projectId, )), - groupId: featureName, + groupId: params?.groupId ?? featureName, }; } else { /// We don't really have good defaults for the other kinds of known strategies, so return an empty map. @@ -700,8 +700,8 @@ export class FeatureToggleService { } private async parametersWithDefaults( projectId: string, - strategyName: string, featureName: string, + strategyName: string, params: IFeatureStrategy['parameters'] | undefined, ) { return mergeAll([ @@ -716,6 +716,7 @@ export class FeatureToggleService { } private async standardizeStrategyConfig( projectId: string, + featureName: string, strategyConfig: Unsaved, existing?: IFeatureStrategy, ): Promise< @@ -741,8 +742,8 @@ export class FeatureToggleService { parameters = await this.parametersWithDefaults( projectId, + featureName, name, - strategyConfig.featureName!, strategyConfig.parameters, ); if (variants && variants.length > 0) { @@ -777,6 +778,7 @@ export class FeatureToggleService { const standardizedConfig = await this.standardizeStrategyConfig( projectId, + featureName, strategyConfig, ); @@ -920,6 +922,7 @@ export class FeatureToggleService { if (existingStrategy.id === id) { const standardizedUpdates = await this.standardizeStrategyConfig( projectId, + featureName, { ...updates, name: updates.name! }, existingStrategy, ); diff --git a/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts index 0e9c118523..404dd9e3a1 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts @@ -143,8 +143,6 @@ test('Should be able to update existing strategy configuration', async () => { expect(createdConfig.id).toEqual(updatedConfig.id); expect(updatedConfig.parameters).toEqual({ b2b: 'true', - // make sure stickiness is preserved - stickiness: createdConfig.parameters?.stickiness, }); }); @@ -766,9 +764,15 @@ test.each([ ['empty stickiness', { rollout: '100', stickiness: '' }], ['undefined stickiness', { rollout: '100' }], ['undefined parameters', undefined], + [ + 'different group id and stickiness', + { rollout: '100', groupId: 'test-group', stickiness: 'userId' }, + ], + ['different rollout', { rollout: '25' }], + ['empty parameters', {}], ])( - 'Should set project default stickiness when creating a flexibleRollout strategy with %s', - async (description, parameters) => { + 'Should use default parameters when creating a flexibleRollout strategy with %s', + async (description, parameters: { [key: string]: any }) => { const strategy = { name: 'flexibleRollout', parameters, @@ -779,7 +783,20 @@ test.each([ }; const projectId = 'default'; const defaultStickiness = `not-default-${description.replaceAll(' ', '-')}`; - const project = await stores.projectStore.update({ + const expectedStickiness = + parameters?.stickiness === '' + ? defaultStickiness + : (parameters?.stickiness ?? defaultStickiness); + const expectedStrategies = [ + { + parameters: { + groupId: parameters?.groupId ?? feature.name, + stickiness: expectedStickiness, + rollout: parameters?.rollout ?? '100', // default rollout + }, + }, + ]; + await stores.projectStore.update({ id: projectId, name: 'stickiness-project-test', defaultStickiness, @@ -802,14 +819,7 @@ test.each([ }); expect(featureDB.environments[0]).toMatchObject({ - strategies: [ - { - parameters: { - ...parameters, - stickiness: defaultStickiness, - }, - }, - ], + strategies: expectedStrategies, }); // Verify that updating the strategy with same data is idempotent @@ -824,14 +834,7 @@ test.each([ }); expect(featureDBAfterUpdate.environments[0]).toMatchObject({ - strategies: [ - { - parameters: { - ...parameters, - stickiness: defaultStickiness, - }, - }, - ], + strategies: expectedStrategies, }); }, ); diff --git a/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts index feaf1db119..248c017e85 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts @@ -159,94 +159,24 @@ test('Can query for features with namePrefix and tags', async () => { expect(features).toHaveLength(1); }); -describe('strategy parameters default to sane defaults', () => { - test('Creating a gradualRollout strategy with no parameters uses the default for all necessary fields', async () => { - const toggle = await featureToggleStore.create('default', { - name: 'testing-strategy-parameters', - createdByUserId: 9999, - }); - const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ - strategyName: 'flexibleRollout', - projectId: 'default', - environment: DEFAULT_ENV, - featureName: toggle.name, - constraints: [], - sortOrder: 15, - parameters: {}, - }); - expect(strategy.parameters).toEqual({ - rollout: '100', - groupId: toggle.name, - stickiness: 'default', - }); +test('Creating an applicationHostname strategy does not get unnecessary parameters set', async () => { + const toggle = await featureToggleStore.create('default', { + name: 'testing-strategy-parameters-for-applicationHostname', + createdByUserId: 9999, }); - test('Creating a gradualRollout strategy with some parameters, only uses defaults for those not set', async () => { - const toggle = await featureToggleStore.create('default', { - name: 'testing-strategy-parameters-with-some-parameters', - createdByUserId: 9999, - }); - const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ - strategyName: 'flexibleRollout', - projectId: 'default', - environment: DEFAULT_ENV, - featureName: toggle.name, - constraints: [], - sortOrder: 15, - parameters: { - rollout: '60', - stickiness: 'userId', - }, - }); - expect(strategy.parameters).toEqual({ - rollout: '60', - groupId: toggle.name, - stickiness: 'userId', - }); - }); - test('Creating an applicationHostname strategy does not get unnecessary parameters set', async () => { - const toggle = await featureToggleStore.create('default', { - name: 'testing-strategy-parameters-for-applicationHostname', - createdByUserId: 9999, - }); - const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ - strategyName: 'applicationHostname', - projectId: 'default', - environment: DEFAULT_ENV, - featureName: toggle.name, - constraints: [], - sortOrder: 15, - parameters: { - hostnames: 'myfantastichost', - }, - }); - expect(strategy.parameters).toEqual({ + const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ + strategyName: 'applicationHostname', + projectId: 'default', + environment: DEFAULT_ENV, + featureName: toggle.name, + constraints: [], + sortOrder: 15, + parameters: { hostnames: 'myfantastichost', - }); + }, }); - test('Strategy picks the default stickiness set for the project', async () => { - const project = await projectStore.create({ - name: 'customDefaultStickiness', - id: 'custom_default_stickiness', - }); - const defaultStickiness = 'userId'; - await db.rawDatabase.raw( - `UPDATE project_settings SET default_stickiness = ? WHERE project = ?`, - [defaultStickiness, project.id], - ); - const toggle = await featureToggleStore.create(project.id, { - name: 'testing-default-strategy-on-project', - createdByUserId: 9999, - }); - const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ - strategyName: 'flexibleRollout', - projectId: project.id, - environment: DEFAULT_ENV, - featureName: toggle.name, - constraints: [], - sortOrder: 15, - parameters: {}, - }); - expect(strategy.parameters.stickiness).toBe(defaultStickiness); + expect(strategy.parameters).toEqual({ + hostnames: 'myfantastichost', }); }); diff --git a/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts index 76224cd169..e94123353e 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts @@ -980,7 +980,6 @@ test('Can update strategy on feature flag', async () => { expect(defaultEnv.strategies).toHaveLength(1); expect(defaultEnv.strategies[0].parameters).toStrictEqual({ userIds: '1234', - stickiness: 'default', }); }); @@ -1005,7 +1004,6 @@ test('should coerce all strategy parameter values to strings', async () => { expect(defaultEnv.strategies).toHaveLength(1); expect(defaultEnv.strategies[0].parameters).toStrictEqual({ foo: '1234', - stickiness: 'default', }); });