Skip to content

ExPlat: remove "unknown" variation, handle null and not-returned cases#167

Merged
leandroalonso merged 3 commits intodevelopfrom
fix/remove_unknown_and_handle_nil_values
Jan 20, 2021
Merged

ExPlat: remove "unknown" variation, handle null and not-returned cases#167
leandroalonso merged 3 commits intodevelopfrom
fix/remove_unknown_and_handle_nil_values

Conversation

@leandroalonso
Copy link
Copy Markdown
Member

@leandroalonso leandroalonso commented Jan 19, 2021

This PR implements some changes in the AB testing feature based on @withinboredom comments in a previous PR:

  • Remove the unknown variation. The default fallback is control
  • If the experiment has a null value in the JSON, treat it as control
  • If the experiment is not returned in the JSON, treat it as control
  • Removes the other() variation. All variations are effectively a treatment variation, the API was slightly changed:
            switch abTesting.experiment("experiment") {
            case .treatment(let variation) where variation == "variation-name":
                // do something
            case .treatment(_):
                // default variation
            case .control:
                // control
            }

How to test

Code review and green CI should be enough. :)

@leandroalonso leandroalonso self-assigned this Jan 19, 2021
Copy link
Copy Markdown

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but might want to also remove other as well, and just have treatment accept a name for the treatment.

@@ -4,7 +4,6 @@ public enum Variation: Equatable {
case control
case treatment
case other(String)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to do what @renanferrari did on wordpress-mobile/WordPress-FluxC-Android#1845 -- and make Treatment take a string since other() is just a different Treatment (for when we support multi-variate experiments).

@leandroalonso
Copy link
Copy Markdown
Member Author

leandroalonso commented Jan 20, 2021

@jkmassel I have a totally unrelated Mac OS X test failing here (testThatOnlyOneFileIsUploadedSimultaneously).

Running it again doesn't seem to solve (and the test works just fine in my machine™️), any idea how to proceed?

Edit: of course now it worked.

@leandroalonso leandroalonso merged commit ff85cd2 into develop Jan 20, 2021
@leandroalonso leandroalonso deleted the fix/remove_unknown_and_handle_nil_values branch January 20, 2021 13:53
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.

2 participants