feat(experiments): Add experiments support in fxa-shared #3183
Conversation
vbudhram
force-pushed
the
add-experiments-support
branch
from
ea4d1a9
to
f66dfb7
Compare
| */ | ||
| public hash(key: string): number { | ||
| // md5 returns 32 hex bytes, we want 32 bits. 32bits = 8 hex bytes. | ||
| const hash = md5(`${this.name || ''}:${key}`).substr(0, 8); |
There was a problem hiding this comment.
nit: since the name member has a default of empty string, || '' isn't necessary here.
| describe('hash', () => { | ||
| it('returns a 32 bit hash', () => { | ||
| const hashes = new Array(ITERATIONS); | ||
| for (var i = 0; i < ITERATIONS; ++i) { |
| const leeway = Math.floor(ITERATIONS * (LEEWAY_PERCENTAGE / 100)); | ||
| const fiftyPercent = Math.round(ITERATIONS / 2); | ||
| const min = fiftyPercent - leeway; | ||
| const max = fiftyPercent + leeway; |
There was a problem hiding this comment.
nit: These lines are repeated a few times.
| experiment.name = 'oldExperiment'; | ||
| experiment.deprecated = true; |
There was a problem hiding this comment.
name and deprecated are private members. Can they be assigned this way?
There was a problem hiding this comment.
I've updated them to be public. Typescript didn't complain but don't think this will hurt?
There was a problem hiding this comment.
Typescript didn't complain
I'm guessing it's a compile time check and the test files don't go through the compiler at all.
|
|
||
| const GROUPS = ['control', 'treatment']; | ||
| return this.uniformChoice(GROUPS, subject.uuid); | ||
| } |
There was a problem hiding this comment.
nit: from looking at this impl, choose either returns false or a string? Could add that the method declaration in the base class.
There was a problem hiding this comment.
TIL that typescript has union types. Allows function to return boolean or string.
| } | ||
| } | ||
|
|
||
| function checkExperimentDistribution(name, experiment) { |
There was a problem hiding this comment.
An arrow function would make this more consistent with the rest of the file.
vbudhram
force-pushed
the
add-experiments-support
branch
from
f66dfb7
to
b36a610
Compare
vbudhram
force-pushed
the
add-experiments-support
branch
from
b36a610
to
c17e6c7
Compare
|
@chenba Thanks for taking a look at this, I've made the updates your recommended. |
Connect with #3170
Unfortunately, I was not able to 100% move the base experiment in content-server over to our fxa-shared project. Please see the linked issue for the different things I tried.
To spare my sanity I opted to update the experiment file to typecript and pull over the same tests from content-server.
This should be enough for me to build out the experiment parts needed in the auth-server and finish #2595.