Validate branch sizes for all experiment types fixes #1030 by jaredlockhart · Pull Request #1034 · mozilla/experimenter

jaredlockhart · 2019-03-08T18:18:36Z

No description provided.

mythmon · 2019-03-08T20:13:31Z

app/experimenter/experiments/tests/test_forms.py

+        form = ExperimentVariantsAddonForm(
+            request=self.request, data=self.data
+        )
+        self.assertTrue(form.is_valid())


So, I can understand why this test works. On the other hand, the data that makes this test valid and interesting is very far away from this test. I think it would be better to include the experiment sizes data directly in the test, even if it is redundant.

This comment also applies to the other tests, which again rely on data that isn't clear and is far away from the test. I think that this hurts the readability of the tests, which makes it harder to maintain.

peterbe · 2019-03-11T14:13:47Z

There's some repetition now. In the whole test suite that != 100 happens in:

TestExperimentVariantsPrefForm.test_form_invalid_if_sizes_sum_to_more_than_100
TestExperimentVariantsPrefForm.test_form_invalid_if_sizes_sum_to_less_than_100
TestExperimentVariantsAddonForm.test_form_invalid_if_sizes_sum_to_more_than_100
TestExperimentVariantsAddonForm.test_form_invalid_if_sizes_sum_to_less_than_100
TestExperimentVariantsFormSet.test_formset_invalid_if_sizes_sum_to_more_than_100
TestExperimentVariantsFormSet.test_formset_invalid_if_sizes_sum_to_less_than_100
TestExperimentVariantsFormSet.test_empty_branch_size_raises_validation_error

The last one isn't actually about != 100 but actually a test of it being == 0.

IMHO, it's a mistake to even have unit tests against TestExperimentVariantsFormSet since that's not the interface. The form submission goes to TestExperimentVariantsPrefForm or TestExperimentVariantsAddonForm which internally implement ExperimentVariantsFormSet.

peterbe

ExperimentVariantsFormSet.clean is still fundamentally wrong in that it accesses form.cleaned_data before the form is bound.

peterbe

ExperimentVariantsFormSet.clean is still fundamentally wrong in that it accesses form.cleaned_data before the form is bound.

jaredlockhart · 2019-03-11T17:52:42Z

@peterbe I see your point, I was thinking of it as the tests against ExperimentVariantsFormSet are unit tests which test its logic in isolation, and the tests against ExperimentVariantsPrefForm and ExperimentVariantsAddonForm are integration tests. This bug was able to sneak into staging because the unit tests existed but not the integration tests. If you think it's more prudent to reduce the number of redundant tests, I agree we could get away with only the integration tests and not the unit tests.

jaredlockhart · 2019-03-11T17:55:00Z

Also @peterbe with regards to validating before the form is bound, can you be more specific?

peterbe · 2019-03-11T19:15:07Z

with regards to validating before the form is bound, can you be more specific?

I might have been wrong on that. A form instance is bound as soon as any data is passed to it. I.e.

>>> form = Form()
>>> form.is_bound
False
>>> form = Form({'some': 'data'})
>>> form.is_bound
True

However, the form.cleaned_data is not available until form.is_valid().

jaredlockhart · 2019-03-11T20:43:04Z

@peterbe Yes that's true but when validating a formset the forms are individually validated before the formset clean method is called:

"The formset clean method is called after all the Form.clean methods have been called. The errors will be found using the non_form_errors() method on the formset."

https://docs.djangoproject.com/en/2.1/topics/forms/formsets/#custom-formset-validation

jaredlockhart · 2019-03-11T22:35:07Z

@peterbe @mythmon I used my good friend @wolever 's handy package https://github.com/wolever/parameterized to parameterize the tests, and added the explicit setup data to each one to make them more self contained.

mythmon

The parameterized helper looks pretty cool. I think the new versions of the tests are easier to read, and I like that it tests both of the forms concretely. I'd still like to see what @peterbe thinks, since he has the blocking review.

peterbe

Working site > perfect unit tests.

If I understand it correctly, there are two formset classes. ExperimentVariantsPrefFormSet and ExperimentVariantsFormSet. The latter is used for addons, via ExperimentVariantsAddonForm. One is a subset of the other where there's a new parameter called value, right?

The only "dirty" thing is that you're using addons type data for pref form classes (unless I got that criss-crossed) but it's fine.

I'm not sure we need a test for the sum being both less than and more than 100 but not important.

One last thing to consider is; do we have a unit test now for when there's *no data at all. E.g. [del self.data[k] for k in self.data if k.startswith('variants-')]
The sum will become 0 since it's an empty list and that's covered by the <100 methods. Not sure it matters though.

peterbe · 2019-03-12T19:45:27Z

app/experimenter/experiments/tests/test_forms.py

+    ["form_class"],
+    [[ExperimentVariantsAddonForm], [ExperimentVariantsPrefForm]],
+)
+class TestExperimentVariantsFormSet(MockRequestMixin, TestCase):


This name isn't great anymore.

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

@kurtmckee

Validate branch sizes for all experiment types fixes #1030

8a2259d

jaredlockhart requested review from mythmon and peterbe and removed request for mythmon March 8, 2019 18:18

mythmon reviewed Mar 8, 2019

View reviewed changes

peterbe suggested changes Mar 11, 2019

View reviewed changes

Use parameterized to repeat the tests for the subclasses

aa4d91b

mythmon reviewed Mar 12, 2019

View reviewed changes

jaredlockhart requested a review from peterbe March 12, 2019 17:27

peterbe approved these changes Mar 12, 2019

View reviewed changes

jaredlockhart merged commit 0875c96 into master Mar 12, 2019

Conversation

jaredlockhart commented Mar 8, 2019

Uh oh!

mythmon Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

mythmon Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

peterbe commented Mar 11, 2019

Uh oh!

peterbe left a comment

Choose a reason for hiding this comment

Uh oh!

peterbe left a comment

Choose a reason for hiding this comment

Uh oh!

jaredlockhart commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredlockhart commented Mar 11, 2019

Uh oh!

peterbe commented Mar 11, 2019

Uh oh!

jaredlockhart commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredlockhart commented Mar 11, 2019

Uh oh!

mythmon left a comment

Choose a reason for hiding this comment

Uh oh!

peterbe left a comment

Choose a reason for hiding this comment

Uh oh!

peterbe Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

jaredlockhart Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaredlockhart commented Mar 11, 2019 •

edited

Loading

jaredlockhart commented Mar 11, 2019 •

edited

Loading