Split Testing

[rlucioni]

This PR gives us the ability to perform split testing on the LMS. For details, see the documentation. The changes in this PR will require DevOps to perform a migration to create the necessary tables, as well as clearance from them to activate the Django admin site on stage and prod. I'm currently speaking with them about the best way to do this.

The experiments to be performed on the LMS high-level tabs are verified, but have not been formally tested. This is because the experiments can be turned off instantly (without pushing new code), and the best-performing variation will be thoroughly tested before being added permanently to the codebase.

@cpennington and @singingwolfboy, could you please help me review this? (N.B. Although my internship ends on August 30, I'm continuing with edX on a limited hours basis during the school year, so this doesn't need to be reviewed and merged right this minute.)

[singingwolfboy]

Why import waffle if it isn't used in this file?

[rlucioni]

I've addressed all comments except for @cpennington's note about the signature of tab generators. As I wrote above, should I still make this change given that use of the request argument by the _courseware tab generator won't be permanent? Once we conclude split testing on the tabs, the request argument won't be needed anymore.

[rlucioni]

Arguments to the tab generators are now uniform, as per our earlier discussion. I'm going to hold off on merging this in until @dianakhuang has Paid Certificates sorted out. In the meantime, I'll be speaking with Devops about changes that need to be made to support this PR. 👍?

[cpennington]

OK, I know this isn't your test, but...

Change self.mockuser0 and self.mockuser1 to actually indicate what's different about them (self.user and self.anonymous_user, or something?) Likewise for self.active_page*. I would also pull out the constant bits of the tab construction process into a helper method that just takes the variables that you want to vary (or takes all the variables, but defaults them to a standard set), to minimize the code on the assertion lines to just the things that are changing.

That will have a happy side effect of making it so that you only have to add FAKE_REQUEST in one place.

[yarko]

This public pull request refers to non-public documentation!?!?

Really - somebody's got to fix that.

[rlucioni]

@yarko, thank you for pointing that out. I'll get that documentation moved.

[rlucioni]

@yarko, I've moved the internal documentation to a page in our GitHub Wiki. Please find the public documentation for split testing here. I've updated the link above to reflect this. Thanks again for catching this.

[rlucioni]

@cpennington, I've refactored the tab tests as you suggested. Any other comments? @singingwolfboy?

[singingwolfboy]

Looks like this line should be removed.

[singingwolfboy]

@rlucioni You've got a failing unit test:

 1) ERROR: test_validate_tabs (courseware.tests.test_tabs.ValidateTabsTestCase)

    Traceback (most recent call last):
     lms/djangoapps/courseware/tests/test_tabs.py line 275 in test_validate_tabs
       self.assertIsNone(tabs.validate_tabs(self.courses[0]))
     lms/djangoapps/courseware/tabs.py line 283 in validate_tabs
       raise InvalidTabsException("Expected at least two tabs.  tabs: '{0}'".format(tabs))
    InvalidTabsException: Expected at least two tabs.  tabs: '<MagicMock name='mock.tabs' id='221364688'>'

Can you fix that? Once the tests pass, the only other thing you need is an entry in the CHANGELOG, and then I believe this is ready to merge.

[rlucioni]

Done. @singingwolfboy and @cpennington?

[singingwolfboy]

👍

[cpennington]

Python should be indented 4 spaces

[cpennington]

Couple of nitpicks, but once those are cleared up: 👍

[yarko]

On Sep 18, 2013 1:06 PM, "Renzo Lucioni" notifications@github.com wrote:

@yarko, I've moved the internal documentation to a page in our GitHub
Wiki. Please find the public documentation for split testing here. I've
updated the link above to reflect this. Thanks again for catching this.

+1

—
Reply to this email directly or view it on GitHub.