Reset schema 911831

[willkg]

To test, I did something like this:

1. run vendor/src/schematic/schematic migrations/ to do the last schematic migration
2. run ./manage.py migrate which should be a no-op
3. change the database name in your settings_local.py file to a new one, create the new database and then do ./manage.py syncdb --migrate which is what the hacking howto says to do to create a fresh db now
4. get a new db dump and run the above two things on it

Clearly step 4 takes forever. You can probably skip it since steps 1 and 2 cover it for the most part.

The one thing that's not tested is the deployment script changes. Definitely worth checking the command lines, but otherwise we'll have to wait until this gets to -dev to test.

The one thing this doesn't touch is the admin page that shows the schema version. I need to think about that. We could continue to show the schema version until we dump schematic and additionally show the south_migrationhistory table. Thoughts on that are welcome.

When this lands, I'll write an email that talks about the changes at a high level and highly encourages everyone to spend some quality time with the South tutorial.

One sucky thing about switching to South is that it creates migration code that's not PEP-8 kosher. So if you have a pre-commit hook that prevents you from committing things that aren't PEP-8 kosher, you might want to ditch it.

r?

[willkg]

The failing test is this one:

======================================================================
FAIL: Verify stored visit counts from mocked data.
----------------------------------------------------------------------
Traceback (most recent call last):

File "/home/travis/build/mozilla/kitsune/kitsune/questions/tests/test_models.py", line 535, in test_visit_count_from_analytics
    eq_(3, QuestionVisits.objects.count())
File "/home/travis/build/mozilla/kitsune/vendor/packages/nose/nose/tools.py", line 31, in eq_
    assert a == b, msg or "%r != %r" % (a, b)
AssertionError: 3 != 4

That didn't fail locally. I'm not sure why that failure would be related to my changes. I currently think it's unrelated.

[rlr]

i get that same test fail :-/

[willkg]

Hrm... puzzling. I'll look at the test.

[rlr]

It works!!!!! Need to figure out what's up with that test. I bet it's a bug in the test itself

[mythmon]

I get the same error Travis did with that test. Did you run the test with FORCE_DB=1?

[willkg]

Looking into this now.

[mythmon]

Besides the test failure, this is working for me. I got a fresh database yesterday, so I think that says good things about how this will work on the production db.

[mythmon]

Why do tests fail when south isn't early?

[willkg]

south overrides syncdb and test commands. So it has to go before django-nose at the very least otherwise tests fail. I put it at the top because it seems weird to put it in the middle if it has to go before other things.

I'm game for putting it somewhere else with a better note. What would you suggest?

[mythmon]

I think this is a fine place to put it, I was simply curious about what caused it to fail. "so tests don't fail." is kind of vague. Maybe revise the comment to be something like "south overrides syncdb and test, so it needs to come before any app that uses those."

[willkg]

I'm using q for output debugging because it makes life way easier. So these are q messages.

0.1s test_visit_count_from_analytics: QuestionVisits.objects.count()=0
 0.1s test_visit_count_from_analytics: Question.objects.count()=0
 0.2s test_visit_count_from_analytics: QuestionVisits.objects.count()=0
 0.2s test_visit_count_from_analytics: Question.objects.count()=3
 0.2s reload_from_analytics: question_id, visits, 1)=(1000000, 3, 1)
 0.2s reload_from_analytics: question_id, visits, 2)=(1000000, 3, 2)
 0.2s reload_from_analytics: 
      question_id, visits, 'created')=(1000000, 3, 'created')
 0.2s reload_from_analytics: question_id, visits, 1)=(82L, 42, 1)
 0.2s reload_from_analytics: question_id, visits, 2)=(82L, 42, 2)
 0.2s reload_from_analytics: 
      question_id, visits, 'created')=(82L, 42, 'created')
 0.2s reload_from_analytics: question_id, visits, 1)=(83L, 27, 1)
 0.2s reload_from_analytics: question_id, visits, 2)=(83L, 27, 2)
 0.2s reload_from_analytics: 
      question_id, visits, 'created')=(83L, 27, 'created')
 0.2s reload_from_analytics: question_id, visits, 1)=(84L, 1337, 1)
 0.2s reload_from_analytics: question_id, visits, 2)=(84L, 1337, 2)
 0.2s reload_from_analytics: 
      question_id, visits, 'created')=(84L, 1337, 'created')
 0.2s test_visit_count_from_analytics: QuestionVisits.objects.count()=4
 0.2s test_visit_count_from_analytics: Question.objects.count()=3

That's instrumenting kitsune.questions.models:QuestionVisits.reload_from_analytics().

Essentially with this test, the output suggests creates all four QuestionVisits objects, but it should only be creating three. The one for question id 1000000 should fail with an IntegrityError, but it doesn't. Thus the test fails.

Further, there are no QuestionVisits or Question objects in the db when this test starts. Only three Question objects are created.

Since I started fiddling with this about 20 minutes ago, this test fails every time when run with other tests. When run alone, everything works super--the IntegrityError is kicked up and the test passes.

Still looking into it, but figured I'd mention the above in case it looked familiar to anyone.

[mythmon]

Small nit: the generated files don't have newlines at the end of them. Github complains about that, and so does Git (I think?). If it is easy to edit the template that generates those, that might be nice. If it isn't easy though, we shouldn't worry about it.

[willkg]

It's not hard to add newlines to the end of all the migration files, but ... it would mean we'd need to make sure we do that going forward. The files have a ton of pep-8 issues. git doesn't really complain about them. github shows a red thing, but I'm not sure that's a complaint--seems more like it's just telling you.

I could go either way. If you do think it's prudent to add them, I'll add them and then add a note to the docs.

[mythmon]

I was suggesting that if we can modify the automatic generation process to fix that, we should. Alembic, for example, uses an editable template to make migrations. If we have to manually tweak every migration we make, I don't think it is worth it.

[willkg]

There's a MIGRATION_TEMPLATE defined in south/management/commands/schemamigration.py and south/management/commands/datamigration.py. We could define our own schemamigration and datamigration commands in sumo that extend the schemamigration and datamigration commands and add a newline. I think that's the only way to do it.

https://bitbucket.org/andrewgodwin/south/src/6612130a3287a9e74bfa157d0a03af0a5df467d3/south/management/commands/schemamigration.py?at=default#cl-208

I'm game for doing that. We could (ab)use that for other things, too. Though I'm not sure what. Maybe knock knock jokes at the end of every migration?

[mythmon]

That sounds like more work than fixing a couple new lines is worth. If you want to add knock knock jokes, that would be cool, but I'm fine either way.

[willkg]

Here's a bunch of facts:

1. The test passes pre-South.
2. With the South changes, the test fails only when I do FORCE_DB=1. If I run the tests as part of the whole suite or on its own with an existing test database, it passes.
3. The crux of the problem is the cls.uncached.create(question_id=question_id, visits=visits). For the case where the question_id is for a question that doesn't exist, that should fail with an IntegrityError. It doesn't when it's running on a clean database.
4. If I add some instrumentation to that test, I can explicitly see no questions and no questionvisits when the test starts. It adds three questions--that looks ok. It then adds four questionvisits which is not ok.
5. South lets you create the db for tests via syncdb or via migrations (SOUTH_TESTS_MIGRATE). Switching modes doesn't help.

That's everything I've gathered by fiddling with this.

I have a couple of thoughts.

1. Maybe the way South sets up the db sets some state in the session that causes problems in the first run, but since the session is killed it doesn't cause problems in further runs? Like FORIEGN_KEY_CHECK?
2. I'm missing some important piece of the big picture.

Anyhow, I'm inclined to just code around this for now since it's one tiny part of one test. I'll toss something together tomorrow.

[willkg]

Bah. Thought I tossed a comment in here. ^^^ That rebases against master and adds a few more commits fixing the issues found here.

[mythmon]

Weird error is weird, but the work around seems sane, and Travis is happy.

r+!

(I've started a conversation with TravisCI about why we aren't seeing the lovely green dots of passing tests here.)

[willkg]

@mythmon @rlr Thank you for looking through this!

Landed in master in:

• 85c34f3 [bug 911831] Fix weirdly failing QuestionsVisits test
• 609144f Quell south logging during tests
• cde04df [bug 911831] Update db and migration docs for South
• 0884893 [bug 911831] Update deployment script with South
• fa412aa [bug 911831] Add initial South migrations
• f164938 [bug 911831] Add South 0.8.2

[shuhaowu]

Wait, so i cannot migrate from the old structure to the new one in place and I have to do a new table?

[willkg]

@shuhaowu You can do:

$ ./vendor/src/schematic/schematic migrations
$ ./manage.py migrate

[shuhaowu]

Ah I didn't do the first step as I thought I already had that up to date db.

Thanks