Fix and replace tests

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<IStrategyConfig>,
         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,
             );

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',
+    'Should use default parameters when creating a flexibleRollout strategy with %s',
-    async (description, parameters) => {
+    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: [
+            strategies: expectedStrategies,
-                {
-                    parameters: {
-                        ...parameters,
-                        stickiness: defaultStickiness,
-                    },
-                },
-            ],
         });
 
         // Verify that updating the strategy with same data is idempotent
@@ -824,14 +834,7 @@ test.each([
         });
 
         expect(featureDBAfterUpdate.environments[0]).toMatchObject({
-            strategies: [
+            strategies: expectedStrategies,
-                {
-                    parameters: {
-                        ...parameters,
-                        stickiness: defaultStickiness,
-                    },
-                },
-            ],
         });
     },
 );

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 an applicationHostname strategy does not get unnecessary parameters set', async () => {
-    test('Creating a gradualRollout strategy with no parameters uses the default for all necessary fields', async () => {
+    const toggle = await featureToggleStore.create('default', {
-        const toggle = await featureToggleStore.create('default', {
+        name: 'testing-strategy-parameters-for-applicationHostname',
-            name: 'testing-strategy-parameters',
+        createdByUserId: 9999,
-            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 a gradualRollout strategy with some parameters, only uses defaults for those not set', async () => {
+    const strategy = await featureStrategiesStore.createStrategyFeatureEnv({
-        const toggle = await featureToggleStore.create('default', {
+        strategyName: 'applicationHostname',
-            name: 'testing-strategy-parameters-with-some-parameters',
+        projectId: 'default',
-            createdByUserId: 9999,
+        environment: DEFAULT_ENV,
-        });
+        featureName: toggle.name,
-        const strategy = await featureStrategiesStore.createStrategyFeatureEnv({
+        constraints: [],
-            strategyName: 'flexibleRollout',
+        sortOrder: 15,
-            projectId: 'default',
+        parameters: {
-            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({
             hostnames: 'myfantastichost',
-        });
+        },
     });
-    test('Strategy picks the default stickiness set for the project', async () => {
+    expect(strategy.parameters).toEqual({
-        const project = await projectStore.create({
+        hostnames: 'myfantastichost',
-            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);
     });
 });
 

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',
     });
 });