Code review comment for lp:~allenap/launchpad/handle-status-ro-crash-bug-905853

Revision history for this message
Julian Edwards (julian-edwards) wrote :

<bigjools> allenap: in the tests in your branch, it's probably worth refactoring the bit that sets properties on objects in a r/w transaction
<allenap> bigjools: Erm, which bit?
<allenap> bigjools: Like in test_handleStatus_OK_sets_build_log?
<bigjools> allenap: line 72/83 of the diff
<bigjools> allenap: I suspect we'll need to do that a lot more in the future
<allenap> bigjools: I don't know what a better way would be. I could instead enter read-only mode in each test individually (via a fixture) I guess.
<bigjools> allenap: I was thinking just a test helper
<bigjools> like setattr
<bigjools> but does the whole transactionny thing
<allenap> bigjools: With the removeSecurityProxy thing too I assume.
<bigjools> allenap: no, the caller can do that
<allenap> bigjools: Okay, I think I have a cool way to do that.

review: Approve

« Back to merge proposal