fixes typo where task id was referenced before use

[lundjordan]

I can't use task now that we moved it outside the block it was created. we know what the task id is though as we define it in task_id already

[Callek]

👍 but really interested in how we hit this in prod but no tests caught it

[codecov-io]

Current coverage is 89.35%

Merging #310 into master will decrease coverage by -1.62% as of 4913533

Powered by Codecov

[lundjordan]

every line bar a couple exception raise lines are covered in tests by Archiver with multiple positive and negative testing.

So what happened? We ended up with a slave creating a tracker (and its celery task) while other slaves where just behind the first and also wanted to create such a tracker and task. What we do in this situation is just pass the url the existing task is already running. In between here we fell down the cracks with tests.

This is because for the celery stuff I am using ALWAYS_EAGER so things happen serially. This removes the chance of the above parallel race condition happening. If you are with me still, you could argue that we should mock this out better to force (fake) these race conditions. I also should have hammered staging with more than a handful of runs.

I blame human error on missing logic flow holes in my tests.

[djmitche]

It's hard to see task.id vs. task_id, too. And this code has changed pretty dramatically from patch to patch, so it's difficult to keep up with what kinds of white-box unit tests would be most useful. In the previous revision of this code, task.id was valid in that line.

Happily for all of us, I'm not suggesting branch coverage (which would, indeed, have caught this, but requires probably twice the number of tests!)

Y'know what would be sweet, though, is a static code analyzer that could detect potential NameErrors like this.