Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1467 from fauvel/issue-1467
Browse files Browse the repository at this point in the history 
Allow conversion weight 0 in split tests
  • Loading branch information
@fauvel
fauvel committed Nov 11, 2014
2 parents 03e7892 + fedaa09 commit 92d4510
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 22 deletions.
27 changes: 15 additions & 12 deletions library/CM/Model/Splittest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,19 +177,21 @@ protected function _getContainingCacheables() {
* @throws CM_Exception_Invalid
*/
protected function _setConversion(CM_Splittest_Fixture $fixture, $weight = null) {
$columnId = $fixture->getColumnId();
$fixtureId = $fixture->getId();
if (null === $weight) {
$weight = 1;
}
if ($weight <= 0) {
throw new CM_Exception_Invalid('Weight must be positive or null, `' . $weight . '` given');
CM_Db_Db::update('cm_splittestVariation_fixture',
array('conversionStamp' => time(), 'conversionWeight' => 1),
array('splittestId' => $this->getId(), $columnId => $fixtureId, 'conversionStamp' => null));
} else {
$weight = (float) $weight;
$columnIdQuoted = CM_Db_Db::getClient()->quoteIdentifier($columnId);
CM_Db_Db::exec('UPDATE `cm_splittestVariation_fixture`
SET `conversionStamp` = COALESCE(`conversionStamp`, ?),
`conversionWeight` = `conversionWeight` + ?
WHERE `splittestId` = ? AND ' . $columnIdQuoted . ' = ?',
array(time(), $weight, $this->getId(), $fixtureId));
}
$weight = (float) $weight;
$fixtureId = $fixture->getId();
$columnId = $fixture->getColumnId();

CM_Db_Db::update('cm_splittestVariation_fixture',
array('conversionStamp' => time(), 'conversionWeight' => $weight),
array('splittestId' => $this->getId(), $columnId => $fixtureId));
}

/**
Expand All @@ -208,6 +210,7 @@ protected function _isVariationFixture(CM_Splittest_Fixture $fixture, $variation
*/
protected function _getVariationFixture(CM_Splittest_Fixture $fixture) {
$columnId = $fixture->getColumnId();
$columnIdQuoted = CM_Db_Db::getClient()->quoteIdentifier($columnId);
$fixtureId = $fixture->getId();

$cacheKey = CM_CacheConst::Splittest_VariationFixtures . '_id:' . $fixture->getId() . '_type:' . $fixture->getFixtureType();
Expand All @@ -218,7 +221,7 @@ protected function _getVariationFixture(CM_Splittest_Fixture $fixture) {
SELECT `variation`.`splittestId`, `variation`.`name`
FROM `cm_splittestVariation_fixture` `fixture`
JOIN `cm_splittestVariation` `variation` ON(`variation`.`id` = `fixture`.`variationId`)
WHERE `fixture`.`' . $columnId . '` = ?', array($fixtureId))->fetchAllTree();
WHERE `fixture`.' . $columnIdQuoted . ' = ?', array($fixtureId))->fetchAllTree();
$cacheWrite = true;
}

Expand Down
10 changes: 7 additions & 3 deletions library/CM/Model/SplittestVariation.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,16 @@ public function getSignificance(CM_Model_SplittestVariation $variationWorse) {

$conversionsExpectedA = $rateTotal * $fixturesA / $netRateA;
$conversionsExpectedB = $rateTotal * $fixturesB / $netRateB;
$sigmaExpectedA = sqrt($conversionsExpectedA * (1 - $conversionsExpectedA / $fixturesA));
$sigmaExpectedB = sqrt($conversionsExpectedB * (1 - $conversionsExpectedB / $fixturesB));
$varianceExpectedA = $conversionsExpectedA * (1 - $conversionsExpectedA / $fixturesA);
$varianceExpectedB = $conversionsExpectedB * (1 - $conversionsExpectedB / $fixturesB);

if ($sigmaExpectedA < 3 || $sigmaExpectedB < 3) {
if ($varianceExpectedA < 9 || $varianceExpectedB < 9) {
return null;
}

$sigmaExpectedA = sqrt($varianceExpectedA);
$sigmaExpectedB = sqrt($varianceExpectedB);

if ($conversionsExpectedA - 3 * $sigmaExpectedA < 0 || $conversionsExpectedB - 3 * $sigmaExpectedB < 0) {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion resources/db/structure.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ CREATE TABLE `cm_splittestVariation_fixture` (
`variationId` int(10) unsigned NOT NULL,
`createStamp` int(10) unsigned NOT NULL,
`conversionStamp` int(11) DEFAULT NULL,
`conversionWeight` decimal(10,2) NOT NULL DEFAULT '1.00',
`conversionWeight` decimal(10,2) NOT NULL DEFAULT '0.00',
UNIQUE KEY `userSplittest` (`userId`,`splittestId`),
UNIQUE KEY `requestClientSplittest` (`requestClientId`,`splittestId`),
KEY `splittestId` (`splittestId`),
Expand Down
7 changes: 7 additions & 0 deletions resources/db/update/39.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

if ('1.00' === CM_Db_Db::describeColumn('cm_splittestVariation_fixture', 'conversionWeight')->getDefaultValue()) {
CM_Db_Db::exec('ALTER TABLE cm_splittestVariation_fixture
MODIFY COLUMN conversionWeight decimal(10,2) NOT NULL DEFAULT 0');
CM_Db_Db::update('cm_splittestVariation_fixture', array('conversionWeight' => 0), array('conversionStamp' => null));
}
1 change: 1 addition & 0 deletions tests/library/CM/Model/Splittest/UserTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public function testSetConversionStatic() {
CM_Model_Splittest_User::setConversionStatic('foo', $user1);
$this->assertSame(1, $variation->getConversionCount(true));
CM_Model_Splittest_User::setConversionStatic('foo', $user2, 2.5);
$this->assertSame(2, $variation->getConversionCount(true));
$this->assertSame(1.75, $variation->getConversionRate(true));
}
}
57 changes: 51 additions & 6 deletions tests/library/CM/Model/SplittestVariationTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,63 @@ public function testGetConversionWeight() {
$test->isVariationFixture($user, 'v1');
$test->isVariationFixture($user2, 'v1');
$test->isVariationFixture($user3, 'v1');
$this->assertSame(0, $variation->getConversionCount(true));
$this->assertSame(0.0, $variation->getConversionWeight(true));
$test->setConversion($user, 3.75);
$test->setConversion($user2, 3.29);
$this->assertSame(2, $variation->getConversionCount(true));
$this->assertSame(7.04, $variation->getConversionWeight(true));
$this->assertSame(2.3466666666667, $variation->getConversionRate(true));
$test->setConversion($user, -2);
$this->assertSame(2, $variation->getConversionCount(true));
$this->assertSame(5.04, $variation->getConversionWeight(true));
$this->assertSame(1.68, $variation->getConversionRate(true));

try {
$test->setConversion($user, -2);
$this->fail('Could set Conversion with negative weight');
} catch (CM_Exception_Invalid $e) {
$this->assertTrue(true);
}
$test->delete();
}

public function testGetConversionWeight_SingleConversion() {
$user = CMTest_TH::createUser();

/** @var CM_Model_Splittest_User $test */
$test = CM_Model_Splittest_User::createStatic(array('name' => 'bar', 'variations' => array('v1')));
/** @var CM_Model_SplittestVariation $variation */
$variation = $test->getVariations()->getItem(0);

$test->isVariationFixture($user, 'v1');
$this->assertSame(0, $variation->getConversionCount(true));
$this->assertSame(0.0, $variation->getConversionWeight(true));
$test->setConversion($user);
$this->assertSame(1, $variation->getConversionCount(true));
$this->assertSame(1., $variation->getConversionWeight(true));
$this->assertSame(1., $variation->getConversionRate(true));
$test->setConversion($user);
$this->assertSame(1, $variation->getConversionCount(true));
$this->assertSame(1., $variation->getConversionWeight(true));
$this->assertSame(1., $variation->getConversionRate(true));

$test->delete();
}

public function testGetConversionWeight_MultipleConversions() {
$user = CMTest_TH::createUser();

/** @var CM_Model_Splittest_User $test */
$test = CM_Model_Splittest_User::createStatic(array('name' => 'bar', 'variations' => array('v1')));
/** @var CM_Model_SplittestVariation $variation */
$variation = $test->getVariations()->getItem(0);

$test->isVariationFixture($user, 'v1');
$this->assertSame(0, $variation->getConversionCount(true));
$this->assertSame(0.0, $variation->getConversionWeight(true));
$test->setConversion($user, 1.);
$this->assertSame(1, $variation->getConversionCount(true));
$this->assertSame(1., $variation->getConversionWeight(true));
$this->assertSame(1., $variation->getConversionRate(true));
$test->setConversion($user, 1.);
$this->assertSame(1, $variation->getConversionCount(true));
$this->assertSame(2., $variation->getConversionWeight(true));
$this->assertSame(2., $variation->getConversionRate(true));

$test->delete();
}
Expand Down

0 comments on commit 92d4510

Please sign in to comment.