feat: test multiple fractional flags with shared seed

[colebaileygit]

This PR

• updates test suite to handle fractional op with custom seed

Related Issues

open-feature/flagd#1264

Notes

Follow-up Tasks

How to test

see open-feature/flagd#1266

[colebaileygit]

@beeme1mr Ran into a small snag since the flagd-testbed submodule in flagd main is quite out of date. After syncing the master back to this branch I now see the golang integration test scenarios are not all wired up yet

When testing this change with the the python provider PR, I get an error for the 3 edge case set here:

  Scenario Outline: Fractional operator
    When a string flag with key "fractional-flag" is evaluated with default value "fallback"
    And a context containing a nested property with outer key "user" and inner key "name", with value <name>
    Then the returned value should be <value>
    Examples:
      | name    | value      |
      | "jack"  | "spades"   |
      | "queen" | "clubs"    |
      | "ten"   | "diamonds" |
      | "nine"  | "hearts"   |
      | 3       | "wild"     |

Looks like it used to error and go to default case since the input was not string, but now that we use cat operator it gets cast to string automatically. I'm leaning towards updating this test case but now to verify the flagd master behavior is in sync, we need to spend some time on the test suite as well (and maybe also in other providers)

Alternatively, we can continue basing this PR off of something that is not main, but then we are missing some new test cases esp edge cases so I think that's a non-starter.

EDIT: After hacking together a local test it looks like integer inputs to jsonlogic cat do error for golang, but not for javascript or python. Don't think we can easily fix that inconsistency unless we look at updating the cat operator upstream or overriding in flagd? I'd say the javascript / python behavior is correct personally

[beeme1mr]

EDIT: After hacking together a local test it looks like integer inputs to jsonlogic cat do error for golang, but not for javascript or python. Don't think we can easily fix that inconsistency unless we look at updating the cat operator upstream or overriding in flagd? I'd say the javascript / python behavior is correct personally

It looks like someone opened an issue about this yesterday.

[Kavindu-Dodan]

@colebaileygit could you specify the error that you are seeing ? I checked this from jsonlogic end and diegoholiveira/jsonlogic#77 is applicable only if we use jsonlogic.ApplyInterface with a custom data input. But for flagd's usage, we should get the data input from a json unmarshel, which should still work as Go's json unmarshel use float64 for any number value.

[colebaileygit]

@Kavindu-Dodan Wish I could share a proper test case, but to reproduce I updated the test-harness submodule in flagd to latest from this branch and ran the tests. Since there is more work needed to get the gherkin tests fully wired there, I simply modified one of the tests in core/pkg/evaluator/fractional_test.go to have "email": 3 as input as a quick proxy.

This line:

err = jsonlogic.Apply(bytes.NewReader(targetingBytes), bytes.NewReader(b), &result)

Returns this error:

interface conversion: interface {} is float64, not []interface {}

[Kavindu-Dodan]

@colebaileygit the failure is generated from in operation and not from the cat operation. The error is thrown from here [1] as our second argument is not an array (nor string). (see testcase - https://github.com/open-feature/flagd/blob/main/core/pkg/evaluator/fractional_test.go#L25-L28 )

From JSON logic explanation, there's no restriction on argument however, both in and cat are specified as String operations [2].

I think we should reconsider our fractional tests if we want to change email to a number (3 is not a valid email anyway ;) )

[1] - https://github.com/diegoholiveira/jsonlogic/blob/v3.4.0/jsonlogic.go#L172
[2] - https://jsonlogic.com/operations.html#in-1

[colebaileygit]

@Kavindu-Dodan You're right! I overlooked that if statement.

Good news is in the test-harness suite we don't use if-in logic so that test should actually still be valid. I re-ran a hacked test using 3 and fractional-cat-- it works and also allocates user to one of the variants. That's good news since the implementations do match, and also confirms that this gherkin test is no longer valid with this case: | 3 | "wild" | since it will generate one of the non-default variants now.

[Kavindu-Dodan]

@colebaileygit perfect, I will approve. But I will wait for @toddbaert's review to merge this as he worked a lot with these test suites :)

[diegoholiveira]

@Kavindu-Dodan Wish I could share a proper test case, but to reproduce I updated the test-harness submodule in flagd to latest from this branch and ran the tests. Since there is more work needed to get the gherkin tests fully wired there, I simply modified one of the tests in core/pkg/evaluator/fractional_test.go to have "email": 3 as input as a quick proxy.

This line:

err = jsonlogic.Apply(bytes.NewReader(targetingBytes), bytes.NewReader(b), &result)

Returns this error:

interface conversion: interface {} is float64, not []interface {}

Can you please provide me the json that provoke this error? I'll be glad to fix it on diegoholiveira/jsonlogic. The example provided on diegoholiveira/jsonlogic#77 does not looks right to me (I explain why into the issue).

[Kavindu-Dodan]

@Kavindu-Dodan Wish I could share a proper test case, but to reproduce I updated the test-harness submodule in flagd to latest from this branch and ran the tests. Since there is more work needed to get the gherkin tests fully wired there, I simply modified one of the tests in core/pkg/evaluator/fractional_test.go to have "email": 3 as input as a quick proxy.
This line:

err = jsonlogic.Apply(bytes.NewReader(targetingBytes), bytes.NewReader(b), &result)

Returns this error:

interface conversion: interface {} is float64, not []interface {}

Can you please provide me the json that provoke this error? I'll be glad to fix it on diegoholiveira/jsonlogic. The example provided on diegoholiveira/jsonlogic#77 does not looks right to me (I explain why into the issue).

@diegoholiveira first of all thank you for maintaining the Go jsonlogic library 🙌

The root cause was not due to any bug in the library. It was caused by improper usage of the in operator.

Rule

{"in": ["@faas.com",  {"var": ["email"]} ]}

Context

{"email" : 3}

This is supposed to be failing anyway :)

Anyway, I attempted to support int type handling with PR diegoholiveira/jsonlogic#78. Feel free to have a look when you have time.

[toddbaert]

Thanks all!

Approved. I pushed 2 small changes to fix the spacing, and change the variants in the nested example to be actual cards (ace-of-spades instead of spades-B; I know that's very silly but I really like the theme and I couldn't help myself 😅 ).