Fix AbTest#alternative_for to always prefer persisted participants

[tjgrathwell]

Prior to this fix, stubbing a specific outcome with AbTest#chooses would occasionally set the 'shown' bit on a participant because the code at https://github.com/assaf/vanity/blob/469917acf/lib/vanity/experiment/ab_test.rb#L242 noticed that the persisted participant's alternative differed from the alternative selected by the hashing algorithm in #alternative_for

This would cause problems downstream in the conversion code because #ab_showing returns true on this participant.

Another alternative to fix my specific issue would be to remove the alternative_for(identity) != index check from AbTest#chooses entirely, because I'm not sure what it's for. But it weirds me out in general that #alternative_for might disagree with the values in the database, and I'm not sure where else that could cause problems.

I didn't write a test exposing this behavior because it would probably require finessing the identity of a vanity participant so the hash code goes a specific way, which seemed brittle. But I can backfill one if it's required to get this merged.

[phillbaker]

@tjgrathwell thanks for the PR!

To clarify - were you only seeing this issue during testing on stubbing or in running this code as well? Under what conditions did you notice the problems with conversions?

This code is a bit confusing, I might need a couple of days to read through it and remember how it all works.

[phillbaker]

@tjgrathwell parsing through this - I'm trying to figure how the @use_probabilities could be falsey in certain situations, it seems like moving these two lines up just skips that check?

Were you only seeing this issue during testing on stubbing? Under what conditions did you notice the problems with conversions in a production application?

[tjgrathwell]

@phillbaker @use_probabilities is true when weights are assigned to the experimental alternatives and false otherwise. Which means another workaround for this problem is to just assign weights to the alternatives in every experiment, but that

The specific issues we encountered were only during test, but could also manifest if any VanityParticipant records were created or updated in the DB with a different seen value than the one that would be computed by the MD5/mod strategy.

Which probably only happens if people are manually mucking around outside of Vanity, but it still seems strange to me that #alternative_for will bypass the already-chosen alternative in the DB and instead recompute it using maths.

[phillbaker]

Thanks @tjgrathwell, I think I'm finally understanding:

• you're seeing tests that deal with conversions occasionally fail (flapping)
• because chooses occasionally calls ab_show when it shouldn't
  • because alternative_for(identity) != index

Just double checking - were you able to check if the other condition connection.ab_showing(@id, identity) != index was ever met?

I think I'm okay with the change, I do want to understand more about what's causing the code to behave differently. Any chance you can post a simplified version of the test case that randomly fails?

[bensheldon]

Closing this because the code is identical to #371 and CI is passing on that PR.