On 5/23/13, Branko Čibej <[email protected]> wrote: > On 23.05.2013 23:52, Olemis Lang wrote: >> On 5/23/13, Olemis Lang <[email protected]> wrote: >>> On 5/23/13, Anze Staric <[email protected]> wrote: >>> [...] >>> >>>> When upgrade is run again, it figures, >>>> that it needs to perform all upgrades and tries to add field product >>>> to table ticket, but this fails, because the field is still there, it >>>> was not deleted in reset_db). >>>> >>> ... and ensure that MP upgrade procedure will not be triggered if DB >>> schema is up to date , which represents a bug indeed . >>> >> I'll clarify what I meant to say with the statement above because it >> may be misunderstood . >> >> Ensure that MP upgrade actions will not be performed due to the fact >> that once MP upgrade method is executed it will notice everything is >> up to date . Not doing so represents an error in MP system which has >> to be asserted and reported as an (error | failure) . > > Not sure I quite understand what you're getting at here. In real life, > I'd expect an upgrade to multi-product to be a manual step,
yes > performed > exactly once per BH/Trac installation. yes ... and no ... needs_upgrade method may be invoked on demand to detect everything is ok with target env (... please read below ...) . That's exactly why Trac will stop working automatically if a new plugin requiring an upgrade is installed . In test code , if you decide not to run the upgrade quite often (e.g. setUpClass) then this patch I've been using to test Bloodhound RPC plugin might be helpful . It transforms MultiproductTestCase helpers into class methods whenever possible . https://issues.apache.org/bloodhound/attachment/ticket/509/t509_r1480735_mp_class_test_setup.diff > In other words, there is no need > to (a) detect that this has already happened, that's built-in behavior , so ... yes , this happens under the hood considering that needs_upgrade method is always invoked on demand and might trigger upgrade procedure or not . Notice that both are implemented by MP system so ... yes , it's the responsibility of the plugin to determine whether an upgrade is required or not ; and those checks are the ones I'm talking about . If the environment is upgraded and upgrade command is attempted immediately after then noticing that needs_upgrade returns True is an imminent confirmation of a bug ... so, afaict we agreed since the beginning ; unless I misunderstood your previous comment . > or (b) be able to undo the > upgrade -- that is, downgrade the database. > downgrade is not possible but , especially in test code , it's possible to dispose the whole database (schema + data + ...) and start all over from scratch (that's what I meant in previous messages) . This is a MUST specially when testing upgrade scenarios . I do not know if that's the case . > This is in fact true for /any/ Trac plugin that adds its own tables to > the database; IIUC reset_db will not remove those tables. > exactly what I'm saying is , if it does not remove those tables, then MP version should not cleared in system table either . > An important attribute of any unit test environment is that test cases > must be isolated -- + ... but the most important attribute of the test suite is that test case success could be interpreted as an irrefutable indicator of correct system behavior ... and there's no better way to achieve that than not to inject a patch in a class of the SUT that will not be used in production . If that can be avoided then much better . > in other words, it should not matter in which order > individual test cases are run. + > This implies that each test case must see > the same initial database yes > -- which either means that it has to be a new > database every time, > or that reset_db has to roll back any changes to > the schema. > not necessarily ... what is needed is setUp and tearDown preserving pre-conditions needed for testing . If MP upgrade is one such pre-condition (agreed) and the fact is that reset_db is jeopardizing such expectations by deleting MP version in system table , then what shall be done is to preserve system value after invoking that method (either by overriding it , or by adding an extra step restoring system value in tearDown after reset_db , or by encapsulating all this in helper methods like those in MultiproductTestCase class , or ...) ; rather than hijacking system behavior by monkey patching a class of the SUT , thus potentially leading to unforeseeable side-effects . I'm of the opinion that ConnectionPool must not be patched this way . PS: If default data needs to be restored there's one such helper method in MultiproductTestCase class . -- Regards, Olemis.
