refactor(feature_flags): Migrate function tests to unit tests for Feature Flags #3226
Conversation
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
pull-request-size
bot
added
the
size/XXL
leandrodamascena
changed the title
|
|
No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure. |
github-actions
bot
added
do-not-merge
need-issue
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #3226 +/- ##
========================================
Coverage 95.38% 95.39%
========================================
Files 208 208
Lines 9618 9633 +15
Branches 1764 1766 +2
========================================
+ Hits 9174 9189 +15
Misses 330 330
Partials 114 114 ☔ View full report in Codecov by Sentry. |
leandrodamascena
removed
do-not-merge
need-issue
|
Hey @ryanpudd! Thanks for sending this PR... Starting the review now. |
There was a problem hiding this comment.
Hey @ryanpudd a ton of thanks for taking your time to create these test cases. I was astonished by their coverage and quality. I left some comment here and we can discuss on these possible improvements. Thanks again for your time and work!
|
|
||
| def test_flags_rule_does_not_match(mocker): | ||
| expected_value = True | ||
| mocked_app_config_schema = { |
There was a problem hiding this comment.
This mocked_app_config_schema is used a lot in different test cases, I think it make sense to make it a fixture and reuse it across different test cases. like here
| app_conf_fetcher.get_configuration() | ||
|
|
||
| # THEN raise ConfigurationStoreError error | ||
| assert "AWS AppConfig configuration" in str(err.value) |
There was a problem hiding this comment.
Is this still required after with pytest.raises(ConfigurationStoreError)
|
|
||
|
|
||
| # check rule match for multiple of action types | ||
| def test_flags_conditions_rule_match_multiple_actions_multiple_rules_multiple_conditions(mocker): |
There was a problem hiding this comment.
This test case seems complicated. Do we need to have all 4 asserts in this single test case? I would suggest using @pytest.mark.parametriz like here to make this test case more explicit
| assert toggle == expected_value_fourth_check | ||
|
|
||
|
|
||
| # check a case where the feature exists but the rule doesn't match so we revert to the default value of the feature |
| assert toggle == expected_value | ||
|
|
||
|
|
||
| def test_flags_match_rule_with_key_in_value_action(mocker): |
There was a problem hiding this comment.
The following 4 test cases are pretty similar to me. Could we consider simplify these cases using @pytest.mark.parametrize like here
|
|
||
|
|
||
| # Test not equals | ||
| def test_flags_not_equal_no_match(mocker): |
There was a problem hiding this comment.
The same as above, the following 14 test cases test_flag_(not_eq/gt/lt)_(no_match/match) are pretty similar to me. If we could simplify these test cases using @pytest.mark.parametrize this will make these test cases much simpler and easier to follow.
heitorlessa
added
the
need-customer-feedback
|
Kudos, SonarCloud Quality Gate passed!
|
|
|
Closing as we haven't heard back after the review since Nov 2023, and we want to appreciate everyone's involved time equally; please feel free to reopen if that was missed by accident. |
Issue number: #2060
Summary
Changes
A start at refactoring FeatureFlag tests. This change converts the existing tests to be Unit Tests rather than functional.
The change is primarily to ensure I'm on the right path with this change, follow up PRs will
User experience
No change to the User Experience
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.