feat: add is_feature_enabled to variant response #285
Conversation
tests/unit_tests/test_features.py
Outdated
| assert variant == { | ||
| "enabled": False, | ||
| "name": "disabled", | ||
| "is_feature_enabled": True, |
There was a problem hiding this comment.
Why did we choose is_feature_enabled over feature_enabled?
There was a problem hiding this comment.
It's the name suggested in the issue, and I noticed we also have a variable with this name in the existing codebase.
If you prefer feature_enabled let me know, I can easily make the change 🙂
There was a problem hiding this comment.
Don't know that I would have picked this up but now that we're talking, I'm leaning towards "feature_enabled" rather
UnleashClient/__init__.py
Outdated
| @@ -427,13 +428,14 @@ def get_variant(self, feature_name: str, context: Optional[dict] = None) -> dict | |||
| """ | |||
| context = context or {} | |||
| context.update(self.unleash_static_context) | |||
| disabled_variation = copy.deepcopy(DISABLED_VARIATION) | |||
There was a problem hiding this comment.
Do we need the copy here? This is a constant so we should never be mutating this
There was a problem hiding this comment.
We don't. Addressed in cc102a1
Fun story: This was me being a Python noob and not realizing the del variant["feature_enabled"] on my test_client_specs was actually affecting the DISABLED_VARIANT constant that was being returned, making it so it would no longer have the feature_enabled key, which in turn caused future del variant["feature_enabled"] to fail in the following tests, since the key would no longer be present. I noticed we deepcopied the DISABLED_VARIANT in another place, so I tried that and it worked, which is why we have this copy.
Overall I think it's better to use the dict comprehension like you've shown me on the other comment, so I adapted the test. I also got rid of the other copies as I don't think they are necessary.
There was a problem hiding this comment.
Oooo! Nice spot! Burned by state mutations xD
|
|
||
|
|
||
| @responses.activate | ||
| def test_uc_get_variant_feature_enabled_no_variants(): |
There was a problem hiding this comment.
Can we get away with bootstrapping rather than a mocked response? These sleeps are making the test suite insanely slow and I'm trying to be defensive about making that problem worse. There's some examples in the test_client_specs.py file where we use bootstrap over a mocked response that removes the need for sleeps and HTTP. If we can't then we can't but I gotta ask!
There was a problem hiding this comment.
Addressed in 7c687f0
Might be worth doing the same for the rest of the tests, but out of scope for this PR.
There was a problem hiding this comment.
I'd love to but gosh it's a lot of work. Definitely something on my back burner for when insomnia strikes again
tests/unit_tests/test_features.py
Outdated
| assert variant == { | ||
| "enabled": False, | ||
| "name": "disabled", | ||
| "is_feature_enabled": True, |
There was a problem hiding this comment.
Don't know that I would have picked this up but now that we're talking, I'm leaning towards "feature_enabled" rather
Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>
Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Description
Adds a
is_feature_enabledproperty to the variant response. This can be especially useful to differentiate between:Fixes #284
Type of change
How Has This Been Tested?
Added new tests and/or adapted existing tests to assume the new behavior.
Checklist: