Skip to content
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

Test isEnabled with context #184

Closed
wants to merge 1 commit into from
Closed

Test isEnabled with context #184

wants to merge 1 commit into from

Conversation

gastonfournier
Copy link
Contributor

About the changes

This is just a test to validate #181

This validates that the context is not being dropped.

Looking into the details:

  1. isEnabled with toggle name and context calls isEnabled with toggle name, context and false as defaultSetting:
    default boolean isEnabled(String toggleName, UnleashContext context) {
    return isEnabled(toggleName, context, false);
    }
  2. Certainly the default implementation is dropping the context:
    default boolean isEnabled(String toggleName, UnleashContext context, boolean defaultSetting) {
    return isEnabled(toggleName, defaultSetting);
    }
  3. But that default method is not invoked because the default client overrides the default implementation:
    @Override
    public boolean isEnabled(
    final String toggleName, final UnleashContext context, final boolean defaultSetting) {
    return isEnabled(toggleName, context, (n, c) -> defaultSetting);
    }

Discussion points

  1. The default methods in the interface seem to be shortcuts for testing (FakeUnleash) but they don't provide the proper implementation for a production system that may want to implement Unleash interface (they should remember to implement that method). For being error-prone I consider we should not have a default implementation for Unleash interface, but that's a breaking change and we should discuss it
  2. I get also confused by another method dropping the fallbackAction and it might be better to have a method receiving all the parameters and this should be the only one you should implement: isEnabled(toggleName, context, fallbackAction, defaultSetting); but I need to dig deeper on the meaning of those parameters and if it makes sense to have both.

I'll talk about this with @chriswk who may have more context

@gastonfournier
Copy link
Contributor Author

Closing as this use case should be already covered by client specification tests, in particular https://github.com/Unleash/client-specification/blob/main/specifications/02-user-with-id-strategy.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant