Code review comment for lp:~jtv/pyjuju/maas-anon-urls

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

The review changes exposed a weakness in the test setup: all instances of FakeFileStorageReturning404 used the same Deferred instance (the one that failed with an http not-found error). Once that had failed once, any other test calls using the same fake file storage would set their client status to 404, but would not return/raise (depending on your choice of Twisted execution model) an error. That was enough to make the old code see a 404, but not the new code which relies on these errors.

One instance went away when I patched out a call to get_url that was irrelevant to the test (reducing the number of 404s incurred from two to one). The other required me to hide the fake filestorage factory in another factory. There are probably nicer ways of doing this, such as making it a class, or refactoring the test helpers yet again. For now, I went with the simplest change that sprang to mind.

« Back to merge proposal