Update automated tests to also check evaluation details

[aarsilv]

Eppo Internal:
🎟️ FF-4335 - Update JS Common Tests for Evaluation Details

Motivation and Context

Our JavaScript SDKs have getXXXXXXXDetails() methods, so we should ensure the details are being correctly populated.

Description

Expand eppo-client.spec.ts to include checking details by using data providers.

How has this been documented?

(no documentation needed for adding tests)

How has this been tested?

This is the tests 😛

[aarsilv]

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

[aarsilv]

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

[aarsilv]

whoops past Aaron left this in 😅

[aarsilv]

Now the assignment could be the details flavor as well

[aarsilv]

Bandits response also hase updatedAt (See Eppo source)

[aarsilv]

Here is where we now check the evaluation details

[greghuels]

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', () => {
        // ...
      });
    });

[aarsilv]

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.

[greghuels]

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.

[aarsilv]

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

[greghuels]

Thanks for doing this! Nothing blocking -- just a few suggestions!