feat: strategy variants

[andreas-unleash]

Description

Implements strategy variants based on Client Specification v4.3.0

Strategy variants take priority over feature variants. A feature variant (if present) will be returned if one is not found in the enabled strategy

Fixes # (issue)

Type of change

Please delete options that are not relevant.

• [1] New feature (non-breaking change which adds functionality)

• [1 ] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

• Unit tests
• [1] Spec Tests
• Integration tests / Manual Tests

Checklist:

• [1 ] My code follows the style guidelines of this project
• [1] I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[sonatype-lift]

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

[thomasheartman]

Regarding the failing tests:

FAILED tests/unit_tests/api/test_feature.py::test_get_feature_toggle_etag_present - assert '' == 'W/"730-v0ozr...7rQ5PxkXfjYQ"'

This one I expected to see. I ran into the same one when I was doing python a little while back. @sighphyre and I both couldn't figure out why it fails, but it appears to have been a breaking change in one of our dependencies. What we did then was just to say that this test should be ignored until some date. You could just extend that for a bit, but we should really look into it.

FAILED tests/unit_tests/test_client.py::test_uc_registers_variant_metrics_for_nonexistent_features - TypeError: the JSON object must be str, bytes or bytearray, not NoneType

Now this one worries me a bit more. This is what I was working on previously. It didn't have any issues back then, so it's strange that it should fail now. Does this test also fail on main?

[andreas-unleash]

Regarding the failing tests:

FAILED tests/unit_tests/api/test_feature.py::test_get_feature_toggle_etag_present - assert '' == 'W/"730-v0ozr...7rQ5PxkXfjYQ"'

This one I expected to see. I ran into the same one when I was doing python a little while back. @sighphyre and I both couldn't figure out why it fails, but it appears to have been a breaking change in one of our dependencies. What we did then was just to say that this test should be ignored until some date. You could just extend that for a bit, but we should really look into it.

FAILED tests/unit_tests/test_client.py::test_uc_registers_variant_metrics_for_nonexistent_features - TypeError: the JSON object must be str, bytes or bytearray, not NoneType

Now this one worries me a bit more. This is what I was working on previously. It didn't have any issues back then, so it's strange that it should fail now. Does this test also fail on main?

Thanks for the info on the first one. The second is around the area I have been working, so I will look into it

[thomasheartman]

The changes here look good to me 😄 As long as the tests pass and the client spec is updated, then that's good! I'm guessing that the cases where you get the old variants are the same as they were before, so we don't need new tests for that?

[sighphyre]

I think the null handling needs some work here, there look like a lot of dead paths or strange paths being executed. We shouldn't be checking for None when the result is coming back from a function that can't return None

The overall approach looks fine to me though

[coveralls]

coverage: 96.956% (-0.007%) from 96.963% when pulling ecbf360 on feat/strategy_variants into 15c9000 on main.

[sighphyre]

Still waiting on a patch to the default property that isn't used. Outside of that I'm happy to approve, the null handling looks substantially saner, nice one!

[sighphyre]

LGTM!