On Monday, September 27, 2010, Patrick Altman <[email protected]> wrote: > > On Sep 26, 2010, at 7:44 PM, Russell Keith-Magee wrote: > >> On Mon, Sep 27, 2010 at 6:24 AM, Karen Tracey <[email protected]> wrote: >>> I've run it on Python 2.4 & 2.5 (Ubuntu, sqlite DB) with no problems. >>> >>> I do have some feedback on the @skipIfDB addition: I'd really like if this >>> could be used to distinguish between the different MySQL storage backends. >>> From a very brief look it seems like currently there are a bunch of tests >>> skipped when the DB backend is MySQL, under the assumption that MySQL does >>> not have transaction support. However MySQL does have transaction support >>> when the InnoDB storage engine is used, so it would be nice if these tests >>> were only skipped when the MySQL/MyISAM combination were in use, not when >>> MySQL/InnoDB was used. >>> >>> Similarly there are a bunch of tests that fail or produce errors when the >>> MySQL/InnoDB combination is in use, because that combination does not >>> support forward foreign-key references when loading fixtures. It's not ideal >>> to skip a bunch of tests for function that really should be tested on this >>> combination (aggregates, for example), but for now at least the fixtures >>> used by these tests do not allow them to pass on MySQL/InnoDB, so they might >>> as well just be skipped. I have quit ever trying to run the full Django test >>> suite using MySQL/InnoDB because there are just too many "known failures" >>> there. It would be nice if the addition of @skipIfDB improved that. (For >>> that matter I also never run the full suite with MySQL/MyISAM because the >>> lack of transaction support with that combo makes it just too slow to run >>> the full suite, but I don't know of any way to improve that situation.) >>> >>> One difficulty in adding this extra check is that it is hard to know for >>> sure what storage engine is in use. If the database definition dict includes >>> 'OPTIONS': { "init_command": "SET storage_engine=<whatever>" } then we know >>> the <whatever> engine is in use. If not, the default configured engine for >>> the MySQL server will be used, and that could be either one. I'd be inclined >>> to say if you want to skip tests based on a particular storage engine being >>> used, then you need to make it explicit in the test settings what engine you >>> are using. So I'd be happy if this @skipIfDB threw an error if it was asked >>> to skip based on storage engine but could not find in the test settings what >>> storage engine was being used. >> >> The problem with this approach is that you can configure MySQL to use >> InnoDB by default for table creation, so there won't necessarily be >> any Django settings providing evidence that transactions are >> available. >> >>> Another difficulty is figuring out how to nicely specify this additional >>> requirement. Have not given that a whole lot of thought yet. Right now the >>> @skipIfDB takes a simple string (matched against DB ENGINE key) or iterable >>> of strings. Supporting querying both ENGINE and OPTIONS, and allowing for >>> more than one to be listed, might get a bit ugly. I am curious why the >>> iterable option here was added -- are there a lot of cases in the current >>> suite where tests need to be skipped on a set of DB backends and not just >>> one? >> >> The use case for the iterable is everywhere that PostgreSQL is the >> skip target, because we have 2 backends that need to be skipped. >> >> I'm in agreement that the ENGINE-string based skipIfDB isn't a >> particularly robust test. If nothing else, it means that if a user >> writes a third-party engine for MySQL (or anything else) in order to >> change some subtle behavior, the test suite won't be any use to them, >> because the string match won't cause the appropriate tests to be >> skipped. >> >> One thought here would be to introduce a variable on the backend >> itself that is a 'db identifier' - i.e., a string that clearly >> identifies the vendor, independent of the path that has been used to >> install the backend. So, for example, a check for >> connection.db_identifier = 'postgresql' would always tell you that >> you're dealing with PostgreSQL, regardless of the import path to the >> actual backend. >> >> This has three potential benefits. >> >> 1. We can avoid the need for the iterI like the idea of connection.features >> for this type of thing. > > In fact, it doesn't seem that there is anything preventing children of > BaseDatabaseFeatures to implement properties instead of just static variables > that actually connects to the configured database connection and queries the > backend specific metadata (e.g. what storage engine am I using?).
Agreed; although there is a mild performance concern if we do that. Hitting the database to determine if a feature is available isn't an expensive operation in itself, but if is repeated a lot of times, the overhead could be considerable. That's not a reason to avoid the technique, of course; just a gotcha to watch out for. Yours, Russ Magee %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
