Skip to content

Update automated tests to also check evaluation details #272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 23, 2025
Merged

Update automated tests to also check evaluation details #272

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
570984c
combine test code for obsfucated and unobfuscated
aarsilv Apr 21, 2025
19de7c3
handle looking at obfuscated rules for details
aarsilv Apr 21, 2025
411b818
tests passing
aarsilv Apr 21, 2025
780c73b
bandit config uses updatedAt
aarsilv Apr 21, 2025
d8ed352
throw if no details when expected
aarsilv Apr 21, 2025
e8e983e
assignment value type
aarsilv Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 40 additions & 73 deletions src/client/eppo-client.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,43 +317,59 @@ describe('EppoClient E2E test', () => {
});
});

describe('UFC Shared Test Cases', () => {
describe.each(['Not Obfuscated', 'Obfuscated'])('UFC Shared Test Cases %s', (obfuscationType) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using dataproviders allows reducing our setup and test case running code duplication

const testCases = testCasesByFileName<IAssignmentTestCase>(ASSIGNMENT_TEST_DATA_DIR);
const isObfuscated = obfuscationType === 'Obfuscated';

describe('Not obfuscated', () => {
beforeAll(async () => {
global.fetch = jest.fn(() => {
return Promise.resolve({
ok: true,
status: 200,
json: () => Promise.resolve(readMockUFCResponse(MOCK_UFC_RESPONSE_FILE)),
});
}) as jest.Mock;
beforeAll(async () => {
global.fetch = jest.fn(() => {
return Promise.resolve({
ok: true,
status: 200,
json: () =>
Promise.resolve(
readMockUFCResponse(
isObfuscated ? OBFUSCATED_MOCK_UFC_RESPONSE_FILE : MOCK_UFC_RESPONSE_FILE,
),
),
});
}) as jest.Mock;

await initConfiguration(storage);
});
await initConfiguration(storage);
});

afterAll(() => {
jest.restoreAllMocks();
});
afterAll(() => {
jest.restoreAllMocks();
});

describe.each(['Scalar', 'With Details'])('%s', (assignmentType) => {
const assignmentWithDetails = assignmentType === 'With Details';

it.each(Object.keys(testCases))('test variation assignment splits - %s', async (fileName) => {
const { flag, variationType, defaultValue, subjects } = testCases[fileName];
const client = new EppoClient({ flagConfigurationStore: storage });
const client = new EppoClient({ flagConfigurationStore: storage, isObfuscated });
client.setIsGracefulFailureMode(false);

let assignments: {
subject: SubjectTestCase;
assignment: string | boolean | number | null | object;
}[] = [];

const typeAssignmentFunctions = {
[VariationType.BOOLEAN]: client.getBooleanAssignment.bind(client),
[VariationType.NUMERIC]: client.getNumericAssignment.bind(client),
[VariationType.INTEGER]: client.getIntegerAssignment.bind(client),
[VariationType.STRING]: client.getStringAssignment.bind(client),
[VariationType.JSON]: client.getJSONAssignment.bind(client),
};
const typeAssignmentFunctions = assignmentWithDetails
Copy link
Contributor

@greghuels greghuels Apr 22, 2025 •
edited by aarsilv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I've found that having some code repetition in your tests is easier to work with than using parameterized tests with conditionals. It makes tests easier to grok and debug and it reduces the chances of having a bug in your test itself.

I would've done this:

    describe.each(Object.keys(testCases))('test variation assignment splits - %s', async (fileName) => {
      it('should validate for Scalar assignment types', () => {
        const { flag, variationType, defaultValue, subjects } = testCases[fileName];
        const client = new EppoClient({ flagConfigurationStore: storage, isObfuscated });
        client.setIsGracefulFailureMode(false);

        let assignments: {
          subject: SubjectTestCase;
          assignment: string | boolean | number | null | object;
        }[] = [];

        const scalarTypeAssignmentFunctions = {
          [VariationType.BOOLEAN]: client.getBooleanAssignment.bind(client),
          [VariationType.NUMERIC]: client.getNumericAssignment.bind(client),
          [VariationType.INTEGER]: client.getIntegerAssignment.bind(client),
          [VariationType.STRING]: client.getStringAssignment.bind(client),
          [VariationType.JSON]: client.getJSONAssignment.bind(client),
        };

        const assignmentFn = scalarTypeAssignmentFunctions[variationType] as (
          flagKey: string,
          subjectKey: string,
          subjectAttributes: Record<string, AttributeType>,
          defaultValue: boolean | string | number | object,
        ) => never;
        if (!assignmentFn) {
          throw new Error(`Unknown variation type: ${variationType}`);
        }

        assignments = getTestAssignments(
          { flag, variationType, defaultValue, subjects },
          assignmentFn,
        );

        validateTestAssignments(assignments, flag, false, isObfuscated);
      });

      it('should validate assignment details', () => {
        // ...
      });
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually started out with this and then decided there was so much repeat code I should pull it out into a private helper method. And then, while parameterizing things to do that, I decided it was easiest to just have the 2x2 grid of obfuscated/not and scalar/details.

I generally am fine with repeat code to make tests easier to understand what is happening, but I think the describe() arrays are not much different than the separate it()s 🤷‍♂️

I also thought having two different validateTestAssignments() methods would be easier and started down that path before deciding to collapse them into one as well.

So basically I had the same instincts as you but after coding up I ended up deciding this was the cleanest. Totally subjective I know. And I do hate how Webstorm chokes on .each() in tests.

? {
[VariationType.BOOLEAN]: client.getBooleanAssignmentDetails.bind(client),
[VariationType.NUMERIC]: client.getNumericAssignmentDetails.bind(client),
[VariationType.INTEGER]: client.getIntegerAssignmentDetails.bind(client),
[VariationType.STRING]: client.getStringAssignmentDetails.bind(client),
[VariationType.JSON]: client.getJSONAssignmentDetails.bind(client),
}
Comment on lines +363 to +370
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now also test the Details() flavors of our methods 📈

: {
[VariationType.BOOLEAN]: client.getBooleanAssignment.bind(client),
[VariationType.NUMERIC]: client.getNumericAssignment.bind(client),
[VariationType.INTEGER]: client.getIntegerAssignment.bind(client),
[VariationType.STRING]: client.getStringAssignment.bind(client),
[VariationType.JSON]: client.getJSONAssignment.bind(client),
};

const assignmentFn = typeAssignmentFunctions[variationType] as (
flagKey: string,
Expand All @@ -370,56 +386,7 @@ describe('EppoClient E2E test', () => {
assignmentFn,
);

validateTestAssignments(assignments, flag);
});
});

describe('Obfuscated', () => {
beforeAll(async () => {
global.fetch = jest.fn(() => {
return Promise.resolve({
ok: true,
status: 200,
json: () => Promise.resolve(readMockUFCResponse(OBFUSCATED_MOCK_UFC_RESPONSE_FILE)),
});
}) as jest.Mock;

await initConfiguration(storage);
});

afterAll(() => {
jest.restoreAllMocks();
});

it.each(Object.keys(testCases))('test variation assignment splits - %s', async (fileName) => {
const { flag, variationType, defaultValue, subjects } = testCases[fileName];
const client = new EppoClient({ flagConfigurationStore: storage, isObfuscated: true });
client.setIsGracefulFailureMode(false);

const typeAssignmentFunctions = {
[VariationType.BOOLEAN]: client.getBooleanAssignment.bind(client),
[VariationType.NUMERIC]: client.getNumericAssignment.bind(client),
[VariationType.INTEGER]: client.getIntegerAssignment.bind(client),
[VariationType.STRING]: client.getStringAssignment.bind(client),
[VariationType.JSON]: client.getJSONAssignment.bind(client),
};

const assignmentFn = typeAssignmentFunctions[variationType] as (
flagKey: string,
subjectKey: string,
subjectAttributes: Record<string, AttributeType>,
defaultValue: boolean | string | number | object,
) => never;
if (!assignmentFn) {
throw new Error(`Unknown variation type: ${variationType}`);
}

const assignments = getTestAssignments(
{ flag, variationType, defaultValue, subjects },
assignmentFn,
);

validateTestAssignments(assignments, flag);
validateTestAssignments(assignments, flag, assignmentWithDetails, isObfuscated);
});
});
});
Expand Down
1 change: 0 additions & 1 deletion src/evaluator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ export class Evaluator {
configDetails.configFormat,
);
} catch (err: any) {
console.error('>>>>', err);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops past Aaron left this in 😅

const flagEvaluationDetails = flagEvaluationDetailsBuilder.gracefulBuild(
'ASSIGNMENT_ERROR',
`Assignment Error: ${err.message}`,
Expand Down
1 change: 1 addition & 0 deletions src/http-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export interface IUniversalFlagConfigResponse {
}

export interface IBanditParametersResponse {
createdAt: string; // ISO formatted string
bandits: Record<string, BanditParameters>;
}

Expand Down
104 changes: 77 additions & 27 deletions test/testHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,16 @@ export function getTestAssignments(
subjectAttributes: Record<string, AttributeType>,
defaultValue: string | number | boolean | object,
) => never,
): { subject: SubjectTestCase; assignment: string | boolean | number | null | object }[] {
): {
subject: SubjectTestCase;
assignment:
| string
| boolean
| number
| null
| object
| IAssignmentDetails<typeof testCase.defaultValue>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the assignment could be the details flavor as well

}[] {
const assignments: {
subject: SubjectTestCase;
assignment: string | boolean | number | null | object;
Expand All @@ -104,48 +113,89 @@ export function getTestAssignments(
return assignments;
}

export function getTestAssignmentDetails(
testCase: IAssignmentTestCase,
assignmentDetailsFn: (
flagKey: string,
subjectKey: string,
subjectAttributes: Record<string, AttributeType>,
defaultValue: string | number | boolean | object,
) => never,
): {
subject: SubjectTestCase;
assignmentDetails: IAssignmentDetails<string | boolean | number | object>;
}[] {
return testCase.subjects.map((subject) => ({
subject,
assignmentDetails: assignmentDetailsFn(
testCase.flag,
subject.subjectKey,
subject.subjectAttributes,
testCase.defaultValue,
),
}));
}
const configCreatedAt = readMockUFCResponse(MOCK_UFC_RESPONSE_FILE).createdAt;
const testHelperInstantiationDate = new Date();

export function validateTestAssignments(
assignments: {
subject: SubjectTestCase;
assignment: string | boolean | number | object | null;
assignment:
| string
| boolean
| number
| object
| null
| IAssignmentDetails<string | boolean | number | object>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: there seems like a bunch of places we could DRY this up a bit.

Could do something like this at the top of the file: type VariationValue = Variation['value'] | object;

Then here you would do: assignment: VariationValue | null | IAssignmentDetails<VariationValue>

There are a few other places in this file that could be replaced with VariationValue if you do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GREAT call--will make that change! We don't even need null it's covered by object

}[],
flag: string,
withDetails: boolean,
isObfuscated: boolean,
) {
for (const { subject, assignment } of assignments) {
if (!isEqual(assignment, subject.assignment)) {
let assignedVariation = assignment;
let assignmentDetails: IFlagEvaluationDetails | null = null;
if (
withDetails === true &&
typeof assignment === 'object' &&
assignment !== null &&
'variation' in assignment
) {
assignedVariation = assignment.variation;
assignmentDetails = assignment.evaluationDetails;
}

if (!isEqual(assignedVariation, subject.assignment)) {
// More friendly error message
console.error(
`subject ${subject.subjectKey} was assigned ${JSON.stringify(
assignment,
assignedVariation,
undefined,
2,
)} when expected ${JSON.stringify(subject.assignment, undefined, 2)} for flag ${flag}`,
);
}

expect(assignment).toEqual(subject.assignment);
expect(assignedVariation).toEqual(subject.assignment);

if (withDetails && assignmentDetails) {
expect(assignmentDetails.environmentName).toBe(subject.evaluationDetails.environmentName);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is where we now check the evaluation details

expect(assignmentDetails.flagEvaluationCode).toBe(
subject.evaluationDetails.flagEvaluationCode,
);
expect(assignmentDetails.flagEvaluationDescription).toBe(
subject.evaluationDetails.flagEvaluationDescription,
);
expect(assignmentDetails.variationKey).toBe(subject.evaluationDetails.variationKey);
// Use toString() to handle comparing JSON
expect(assignmentDetails.variationValue?.toString()).toBe(
subject.evaluationDetails.variationValue?.toString(),
);
expect(assignmentDetails.configPublishedAt).toBe(configCreatedAt);
// cannot do an exact match for configFetchedAt because it will change based on fetch
expect(new Date(assignmentDetails.configFetchedAt).getTime()).toBeGreaterThan(
testHelperInstantiationDate.getTime(),
);

if (!isObfuscated) {
expect(assignmentDetails.matchedRule).toEqual(subject.evaluationDetails.matchedRule);
} else {
// When obfuscated, rules may be one-way hashed (e.g., for ONE_OF checks) so cannot be unobfuscated
// Thus we'll just check that the number of conditions is equal and relay on the unobfuscated
// tests for correctness
expect(assignmentDetails.matchedRule?.conditions || []).toHaveLength(
subject.evaluationDetails.matchedRule?.conditions.length || 0,
);
}

expect(assignmentDetails.matchedAllocation).toEqual(
subject.evaluationDetails.matchedAllocation,
);
expect(assignmentDetails.unmatchedAllocations).toEqual(
subject.evaluationDetails.unmatchedAllocations,
);
expect(assignmentDetails.unevaluatedAllocations).toEqual(
subject.evaluationDetails.unevaluatedAllocations,
);
}
}
}
Loading