ExPlat: Handle local Experiment registration

[renanferrari]

This PR is a follow up to #14498 and is related to wordpress-mobile/WordPress-FluxC-Android#1970.

These changes can be divided into two parts:

• Part 1: Handling Experiment registration in the ExPlat class.
• Part 2: Providing a set of Experiments to the ExPlat class for registration.

I thought about splitting this PR in two, but its changes are not so big overall and I think they make sense together, but let me know what you prefer.

Part 1

In this first part, the ExPlat class was updated in two ways:

• It now has a start() method that replaces the direct call we previously made to forceRefresh() once the app started. This method accepts a set of Experiments, maps it to a list of experiment names which is then held internally and then calls forceRefresh().
• The list of experiment names is then used so that we can check when we should retrieve assignments:
  • For forceRefresh() and refreshIfNeeded(), we check if the list is empty and if so, we prevent any calls to the API from being made.
  • For getVariation() we check if the list contains the name of the provided Experiment and if it doesn't, we just return Control directly (if BuildConfig.DEBUG is true, we throw a IllegalArgumentException instead).

Part 2

In this part, Dagger is being used to actually provide the set of Experiments to the ExPlat class. Since our Experiment implementations were already being included in the dependency graph, I just created a ExperimentModule where we can add the experiments we want to register and bind them all into a Set<Experiment> by using a combination of @Binds and @IntoSet annotations. It also uses a @Multibinds annotation to provide an empty Set in case no Experiment is currently registered.

This approach is a bit similar to the one we use to provide a Map of ViewModels to the ViewModelFactory class and it introduces a similar extra step. To demonstrate, here's how it would work in practice to add a new Experiment:

1. We create a new class that extends from the Experiment class, and inject the ExPlat dependency using Dagger, just like we did before:

class ExampleExperiment @Inject constructor(exPlat: ExPlat) : Experiment("example_experiment", exPlat)

2. (new step) We then "register" this Experiment in the ExperimentModule like so:

@Binds @IntoSet fun exampleExperiment(experiment: ExampleExperiment): Experiment

And that's it. I also created an ExampleExperiment and left a commented out line in ExperimentModule with the code above as an example.

👉 Note: while this approach is pretty safe to use, I don't expect it to be final. Once we start working on adding functionality to list and edit registered experiments locally (as described in #14089), I expect we should be able to replace it with something similar to what we currently have with our remote configs, where we would just need to add an annotation to our Experiment class and the generated code would handle the rest for us. For now, other than that extra step, I don't see any major drawbacks that would prevent us from moving forward with this, so please let me know if you do!

To test

Without registered experiments

1. Open the app.
2. On logcat, make sure you don't see any messages like: ExPlat: fetching assignments successful with result: ....

Try logging in/out and restarting the app and make sure you still don't see the message.

With registered experiments

1. Uncomment the commented line in the ExperimentModule class and restart the app.
2. Open the app.
3. On logcat, notice a message like: ExPlat: fetching assignments successful with result: ....

Regression Notes

1. Potential unintended areas of impact
   n/a

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   n/a

3. What automated tests I added (or what prevented me from doing so)
   Added a few extra unit tests to account for new behaviors.

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[planarvoid]

I'm wondering whether it would make sense to inject this directly in the ExPlat class. The reason is that it would hide this internal piece of logic in there instead of using it in the shared application class. I see it's a singleton but I wouldn't expect it to outlive the application class. WDYT?

[renanferrari]

That would make sense, for sure. The reason I avoided doing that in the first place was because I didn't want to generate a circular dependency (ExPlat depends on Experiment that depends on ExPlat).

That said, I tried using dagger.Lazy to inject the Set<Experiment> in the ExPlat constructor and it actually ended up working pretty well and the code got cleaner, specially because we no longer need the ExPlat::start method.

You can check the changes here: 3eb27b7 – thanks for the suggestion!

[planarvoid]

Is it viable to introduce some kind of a lint rule that would fail if there is an experiment that is not registered in the module? Maybe that's unnecessary.

[renanferrari]

This is probably viable, although I have no experience with custom lint rules so I can't say for sure.

That said, if that's something we want to do, we should probably do it in a different PR (specially since I guess it would need to be submitted to WordPress-Lint-Android?).

[planarvoid]

It's looking good and works well, thanks for the PR! I have one comment 👍

[planarvoid]

Thanks for the changes, it's looking good 👍