mint-metrics / mojito-js-delivery

🧪 Source-controlled JS split testing framework for building and launching A/B tests.
https://mojito.mx/docs/js-delivery-intro
Other
16 stars 29 forks source link

Report tests in container build script output #6 issue #15

Closed allmywant closed 4 years ago

allmywant commented 4 years ago

Print the IDs of tests included in the container when the build script runs

kingo55 commented 4 years ago

@allmywant @dapperdrop - just been testing out this PR here on one of our containers. Working flawlessly...

Screen Shot 2019-10-21 at 9 13 05 pm

Should we differentiate divertTo tests in live? They're technically live, but not tracking etc.

allmywant commented 4 years ago

@kingo55

@allmywant @dapperdrop - just been testing out this PR here on one of our containers. Working flawlessly...

Screen Shot 2019-10-21 at 9 13 05 pm

Should we differentiate divertTo tests in live? They're technically live, but not tracking etc.

Yes, I think we should do that. And the condition should be: state is staging and devertTo isn't null.

dapperdrop commented 4 years ago

Yes, I think we should do that. And the condition should be: state is staging and devertTo isn't null.

Should it just test for divertTo not null? Sometimes I have state is live with divertTo set, and the experiment is diverted.

allmywant commented 4 years ago

Should it just test for divertTo not null? Sometimes I have state is live with divertTo set, and the experiment is diverted.

You're right, I can do it like:

if (test.state == 'live')
{
     liveList.push(test.name);
}
else if (test.state == 'staging' && test.divertedTo != null)
{
     liveList.push(test.name);
}
dapperdrop commented 4 years ago

Should it just test for divertTo not null? Sometimes I have state is live with divertTo set, and the experiment is diverted.

You're right, I can do it like:

if (test.state == 'live')
{
     liveList.push(test.name);
}
else if (test.state == 'staging' && test.divertedTo != null)
{
     liveList.push(test.name);
}

I'm thinking maybe moving the divertTo check above other checks, e.g.

if (test.divertedTo != null)
{
     divertList.push(test.name);
}
else if (test.state == 'live')
{
     liveList.push(test.name);
}
else if (test.state == 'staging')
{
    stagingList.push(test.name);
}

etc...
allmywant commented 4 years ago

You're right @dapperdrop So, we just need to count/list the divertTo tests in live, add a new category divertTo?

dapperdrop commented 4 years ago

Yes that's right @allmywant - basically another list:

Live (x) -... Staging (x) -... Diverted (x) -... Inactive (x)

allmywant commented 4 years ago

Diverted list added.

kingo55 commented 4 years ago

Excellent! This is even clearer now.

➜  med git:(development) ✗ gulp scripts && gulp publish                                                   
[17:23:47] Using gulpfile ~/Documents/med/gulpfile.js
[17:23:47] Starting 'scripts'...
[17:23:47] Finished 'scripts' after 709 ms
Mojito container built with 10 tests (24.39 KB):
  Live (2) - w146 w148-2
  Staging (2) - w143 w149
  Diverted (6) - W122 w139 w141 w142 w146-divert w147
  Inactive (6)

To confirm, staging experiments can't be divertTo experiments, right? If an experiment is in staging then divertTo gets ignored, right? In that case, shouldn't the logic be:

if (test.state == 'live')
{
    if (test.divertedTo != null)
    {
        divertList.push(test.name);
    }
    else 
    {
        liveList.push(test.name);
    }
}
else
{
    stagingList.push(test.name);
}
allmywant commented 4 years ago

Just read related source code, divertTo works in both staging and live experiments.

kingo55 commented 4 years ago

Just read related source code, divertTo works in both staging and live experiments.

Ah you're right @allmywant - That doesn't sound like the expected behaviour for divertTo. I'll make a PR to fix that, so only live tests should divert, I think. You typically think of staging as not live.

@dapperdrop is this your understanding of divert?

dapperdrop commented 4 years ago

Agreed, @kingo55 divertTo should only be functional if the test is live.

Thanks for fixing it in the PR.

kingo55 commented 4 years ago

@allmywant - I just merged in the divertTo behaviour change. Would you mind updating the build script's logic to match up, please?

if (test.state == 'live')
{
    if (test.divertedTo != null)
    {
        divertList.push(test.name);
    }
    else 
    {
        liveList.push(test.name);
    }
}
else
{
    stagingList.push(test.name);
}
allmywant commented 4 years ago

@kingo55 Updated.

kingo55 commented 4 years ago

Awesome, thanks @allmywant !