I have a few additional notes that Gavin thought were worth adding:
* What did the pre-imp conclude about the problem of re-enabling archives? Is it not going to be a problem to un-suspend jobs when that happens?
* In build_not_pending_and_suspended, the "not" is ambiguous in scope. Can't you replace this with two separate checks?
* In production code we check "len(foo) == 0" to check for empty lists. But in tests it's nicer to compare "foo == []" so that a failure will give more information.
* With assertEqual, pass expected value first and actual value second! Again, better failure messages.
I have a few additional notes that Gavin thought were worth adding:
* What did the pre-imp conclude about the problem of re-enabling archives? Is it not going to be a problem to un-suspend jobs when that happens?
* In build_not_ pending_ and_suspended, the "not" is ambiguous in scope. Can't you replace this with two separate checks?
* In production code we check "len(foo) == 0" to check for empty lists. But in tests it's nicer to compare "foo == []" so that a failure will give more information.
* With assertEqual, pass expected value first and actual value second! Again, better failure messages.
Jeroen