Skip to content

Validate branch sizes for all experiment types fixes #1030 #1034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jaredlockhart merged 2 commits into from
Mar 12, 2019
Merged

Validate branch sizes for all experiment types fixes #1030 #1034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a2259d
Validate branch sizes for all experiment types fixes #1030
jaredlockhart Mar 8, 2019
aa4d91b
Use parameterized to repeat the tests for the subclasses
jaredlockhart Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/experimenter/experiments/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,11 @@ def clean(self):
form._errors["name"] = ["All branches must have a unique name"]


class ExperimentVariantsPrefFormSet(BaseInlineFormSet):
class ExperimentVariantsPrefFormSet(ExperimentVariantsFormSet):

def clean(self):
super().clean()

alive_forms = [
form
for form in self.forms
Expand Down
88 changes: 57 additions & 31 deletions app/experimenter/experiments/tests/test_forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@

from django import forms
from django.core.exceptions import ValidationError
from django.forms import inlineformset_factory
from django.test import TestCase
from django.utils import timezone
from parameterized import parameterized_class

from experimenter.experiments.forms import (
BugzillaURLField,
Expand All @@ -21,7 +21,6 @@
ExperimentVariantAddonForm,
ExperimentVariantPrefForm,
ExperimentVariantsAddonForm,
ExperimentVariantsFormSet,
ExperimentVariantsPrefForm,
JSONField,
)
Expand Down Expand Up @@ -256,70 +255,97 @@ def test_empty_slug_raises_error(self):
self.assertFalse(form.is_valid())


class TestExperimentVariantsFormSet(TestCase):
@parameterized_class(
["form_class"],
[[ExperimentVariantsAddonForm], [ExperimentVariantsPrefForm]],
)
class TestExperimentVariantsFormSet(MockRequestMixin, TestCase):
Copy link
Copy Markdown
Contributor

@peterbe peterbe Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name isn't great anymore.

Copy link
Copy Markdown
Collaborator Author

@jaredlockhart jaredlockhart Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it applies because that's the name of the class that implements the logic that it's testing.


def setUp(self):
self.FormSet = inlineformset_factory(
formset=ExperimentVariantsFormSet,
model=ExperimentVariant,
parent_model=Experiment,
fields=["is_control", "ratio", "name", "slug", "description"],
super().setUp()

self.experiment = ExperimentFactory.create_with_status(
Experiment.STATUS_DRAFT, num_variants=0
)

self.data = {
"population_percent": "10",
"firefox_version": Experiment.VERSION_CHOICES[-1][0],
"firefox_channel": Experiment.CHANNEL_NIGHTLY,
"client_matching": "en-us only please",
"pref_key": "browser.test.example",
"pref_type": Experiment.PREF_TYPE_STR,
"pref_branch": Experiment.PREF_BRANCH_DEFAULT,
"variants-TOTAL_FORMS": "3",
"variants-INITIAL_FORMS": "0",
"variants-MIN_NUM_FORMS": "0",
"variants-MAX_NUM_FORMS": "1000",
"variants-0-is_control": True,
"variants-0-ratio": "34",
"variants-0-name": "control name",
"variants-0-slug": "control-slug",
"variants-0-description": "control desc",
"variants-0-value": '"control value"',
"variants-1-is_control": False,
"variants-1-ratio": "33",
"variants-1-name": "branch 1 name",
"variants-1-slug": "branch-1-slug",
"variants-1-description": "branch 1 desc",
"variants-1-value": '"branch 1 value"',
"variants-2-is_control": False,
"variants-2-ratio": "33",
"variants-2-name": "branch 2 name",
"variants-2-slug": "branch-2-slug",
"variants-2-description": "branch 2 desc",
"variants-2-value": '"branch 2 value"',
}

def test_formset_valid_if_sizes_sum_to_100(self):
formset = self.FormSet(data=self.data)
self.assertTrue(formset.is_valid())
def test_form_valid_if_sizes_sum_to_100(self):
self.data["variants-0-ratio"] = "34"
self.data["variants-1-ratio"] = "33"
self.data["variants-2-ratio"] = "33"
form = self.form_class(
request=self.request, data=self.data, instance=self.experiment
)
self.assertTrue(form.is_valid())

def test_formset_invalid_if_sizes_sum_to_less_than_100(self):
self.data["variants-0-ratio"] = 30
formset = self.FormSet(data=self.data)
self.assertFalse(formset.is_valid())
def test_form_invalid_if_sizes_sum_to_less_than_100(self):
self.data["variants-0-ratio"] = "33"
self.data["variants-1-ratio"] = "33"
self.data["variants-2-ratio"] = "33"
form = self.form_class(
request=self.request, data=self.data, instance=self.experiment
)
self.assertFalse(form.is_valid())

for form in formset.forms:
for form in form.variants_formset.forms:
self.assertIn("ratio", form.errors)

def test_formset_invalid_if_sizes_sum_to_more_than_100(self):
self.data["variants-0-ratio"] = 40
formset = self.FormSet(data=self.data)
self.assertFalse(formset.is_valid())
def test_form_invalid_if_sizes_sum_to_more_than_100(self):
self.data["variants-0-ratio"] = "35"
self.data["variants-1-ratio"] = "33"
self.data["variants-2-ratio"] = "33"
form = self.form_class(
request=self.request, data=self.data, instance=self.experiment
)
self.assertFalse(form.is_valid())

for form in formset.forms:
for form in form.variants_formset.forms:
self.assertIn("ratio", form.errors)

def test_formset_invalid_if_duplicate_slugs_appear(self):
self.data["variants-0-slug"] = self.data["variants-1-slug"]
formset = self.FormSet(data=self.data)
self.assertFalse(formset.is_valid())
def test_form_invalid_if_duplicate_names_appear(self):
self.data["variants-0-name"] = self.data["variants-1-name"]
form = self.form_class(
request=self.request, data=self.data, instance=self.experiment
)
self.assertFalse(form.is_valid())

for form in formset.forms:
for form in form.variants_formset.forms:
self.assertIn("name", form.errors)

def test_empty_branch_size_raises_validation_error(self):
del self.data["variants-0-ratio"]
formset = self.FormSet(data=self.data)
self.assertFalse(formset.is_valid())
form = self.form_class(
request=self.request, data=self.data, instance=self.experiment
)
self.assertFalse(form.is_valid())


class TestExperimentVariantsAddonForm(MockRequestMixin, TestCase):
Expand Down
3 changes: 3 additions & 0 deletions app/requirements/default.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,6 @@ requests[security]==2.20.0 \
whitenoise==4.1.2 \
--hash=sha256:118ab3e5f815d380171b100b05b76de2a07612f422368a201a9ffdeefb2251c1 \
--hash=sha256:42133ddd5229eeb6a0c9899496bdbe56c292394bf8666da77deeb27454c0456a
parameterized==0.7.0 \
--hash=sha256:020343a281efcfe9b71b9028a91817f981202c14d72104b5a2fbe401dee25a18 \
--hash=sha256:d8c8837fb677ed2d5a93b9e2308ce0da3aeb58cf513120d501e0b7af14da78d5