tests for multiple KPIs, fix bugs found with tests, refs #30

sixpack/models.py

@@ -229,7 +229,7 @@ def convert(self, client, dt=None, kpi=None):
                 if not Experiment.validate_kpi(kpi):
                     raise ValueError('invalid kpi name')
                 self.add_kpi(kpi)
-            alternative.record_conversion(client, dt=dt, kpi=kpi)
+            alternative.record_conversion(client, dt=dt)
 
         return alternative.name
 
@@ -243,7 +243,7 @@ def set_kpi(self, kpi):
         self.kpi = kpi
 
     def add_kpi(self, kpi):
-        self.redis.sadd("{0}:kpis".format(self.key()), kpi)
+        self.redis.sadd("{0}:kpis".format(self.key(include_kpi=False)), kpi)
         self.kpi = kpi
 
     def get_kpis(self):
@@ -605,7 +605,7 @@ def record_participation(self, client, dt=None):
         ]
         msetbit(keys=keys, args=([self.experiment.sequential_id(client), 1] * len(keys)))
 
-    def record_conversion(self, client, dt=None, kpi=None):
+    def record_conversion(self, client, dt=None):
         """Record a user's conversion in a test along with a given variation"""
         if dt is None:
             date = datetime.now()

sixpack/test/experiment_model_test.py

@@ -331,3 +331,42 @@ def test_invalid_kpi(self):
         self.assertFalse(ret)
         ret = Experiment.validate_kpi('&!&&!&')
         self.assertFalse(ret)
+
+    def test_set_kpi(self):
+        exp = Experiment.find_or_create('multi-kpi', ['kpi', '123'], self.redis)
+        # We shouldn't beable to manually set a KPI. Only via web request
+        with self.assertRaises(ValueError):
+            exp.set_kpi('bananza')
+
+        # simulate conversion via webrequest
+        client = Client(100, self.redis)
+        # hack for disabling whiplash
+        exp.random_sample = 1
+
+        exp.get_alternative(client)
+        exp.convert(client, None, 'bananza')
+
+        exp2 = Experiment.find_or_create('multi-kpi', ['kpi', '123'], self.redis)
+        self.assertEqual(exp2.kpi, None)
+        exp2.set_kpi('bananza')
+        self.assertEqual(exp2.kpi, 'bananza')
+
+    def test_add_kpi(self):
+        exp = Experiment.find_or_create('multi-kpi-add', ['asdf', '999'], self.redis)
+        kpi = 'omg-pop'
+
+        exp.add_kpi(kpi)
+        key = "{0}:kpis".format(exp.key(include_kpi=False))
+        self.assertIn(kpi, self.redis.smembers(key))
+        exp.delete()
+
+    def test_get_kpis(self):
+        exp = Experiment.find_or_create('multi-kpi-add', ['asdf', '999'], self.redis)
+        kpis = ['omg-pop', 'zynga']
+
+        exp.add_kpi(kpis[0])
+        exp.add_kpi(kpis[1])
+        ekpi = exp.get_kpis()
+        self.assertIn(kpis[0], ekpi)
+        self.assertIn(kpis[1], ekpi)
+        exp.delete()