Code review comment for lp:~therve/storm/277095-update-cache

Revision history for this message
James Henstridge (jamesh) wrote :

Looks good. You could probably simplify the test a bit: I don't think you need to overlap the two transactions to trigger the bug. The following fails without your changes and passes with your changes, even on sqlite:

        store = self.create_store()
        foo2 = store.get(Foo, 10)
        self.assertEquals(foo2.title, u"Title 30")
        store.commit()

        foo1 = self.store.get(Foo, 10)
        foo1.title = u"Title 40"
        self.store.commit()

        foo2.title = u"Title 30"
        store.commit()
        self.assertEquals(foo2.title, u"Title 30")

Not having to disable the test on sqlite seems worth doing.

review: Approve

« Back to merge proposal