-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: dependant toggles #155
Conversation
This pr implements dependant feature toggles. The tests are specified in the client specifications Unleash/client-specification#63
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some minor comments
Pull Request Test Coverage Report for Build 6531319997
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to metrics? Parent features shouldn't get metrics incremented. Can we add a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
G. I realise we want tests for metrics, this is quite tough to do with current structure. Yggdrasil already has those covered. If we want to move to that faster in lieu of tests here that's.... probably okay
We are not touching the path of metrics here. Metrics behaviour is not tested in the sdk at all. If we want to have test for metrics behaviour I vote for having that as a separate PR. |
@gardleopard the important part is that introducing dependent features shouldn't trigger metrics reported on the parent feature. If that's the case then we're fine. Tests can be added separately. |
This pr implements dependant feature toggles. The tests are specified in the client specifications Unleash/client-specification#63