> - How will this change impact upon other test cases ? Every test case that constructs env in __init__ / setUpClass / globally will work exactly as before. Test cases that construct env in setUp will have a separate in memory database for each test run and will start with an empty database, that will be filled with tables / data in setUp. Currently, first test in suite starts with the empty database and deletes data from it in tearDown. If it modified schema (added columns, tables) this changes are visible for the following test.
> - Is this closer to the code run in production ? In production, each environment is associated with a separate database. As with this patch, we run every test case in a fresh database, this is closer to production, as cases when tables are upgraded, but plugin version is deleted from the system table are avoided. > - ... or introduce false negatives in test reports ? If a hypothetical test assumes to get different connections from the pool (which means it is not using an in-memory database), the patch breaks the assumption. > that's exactly why MP upgrade should be performed once for each unit > test . After doing so , execute assertions on the SUT and get rid of > it afterwards ... the next TC will start from scratch and recreate > everything once again Please note, that reset_db does not downgrade the database, it just removes the data from the tables. If env.upgrade is run again, it fails (multiproduct plugin version is deleted from the system table, but the tables are modified. 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). To minimize the impact of the patch, I am thinking of adding another (optional) parameter fresh_database (or something alike) to (multiproduct.)EnvironmentStub constructor. When set to true, the patch would be applied before parent constructor is called and unapplied afterwards. As the DatabaseManage is created in parent constructor, and it keeps the reference to the connection pool, this should be sufficient. It would require us to manually enable this option for the tests that need it, but it has way less side effects than the current monkey patch. On Thu, May 23, 2013 at 5:01 PM, Olemis Lang <[email protected]> wrote: > On 5/23/13, Anze Staric <[email protected]> wrote: >> On Thu, May 23, 2013 at 2:25 PM, Gary Martin <[email protected]> >> wrote: >>> Apart from this kind of case, I was also thinking that it might be better >>> to >>> get the reset_db method overridden so that it resets the database back to >>> the clean form that we want. >> >> I like the idea, > > it's nice , even if I'd prefer to stick to patch the connection pool > in TC setUp and revert in tearDown > >> but how to we get the database to a clean state after >> it has been migrated to multiproduct in setUp? > > that's exactly why MP upgrade should be performed once for each unit > test . After doing so , execute assertions on the SUT and get rid of > it afterwards ... the next TC will start from scratch and recreate > everything once again > >> Do we hard code which >> fields and tables need to be removed and which keys do need to be >> modified? >> > > Your point is valid . This is a real difficulty . In recent functional > TCs I've written for RPC plugin what I do is to ensure that the names > of the products created for testing will never be the same . Then in > tearDown I just clear bloodhound_product table (except entry for > default product) and isolation across product environments (combined > with GUID for products) will ensure there will be no side-effects > propagated beyond test case lifetime . > > There's no need to do so for unit tests . > >> I still think that forcing the trac to use a separate database for >> separate environments is a cleaner solution. An alternative to monkey >> patching ConnectionPool is to modify EnvironmentStub to use >> DatabaseManager that creates one connection to database per product >> (if we are using in memory database). >> > > I'd be ok with that *if* > > - the patch is really needed > - it will only affect a limited (and relevant) subset of the test suite > > -- > Regards, > > Olemis.
