Opened 12 years ago

Closed 10 years ago

#1746 closed defect (fixed)

Tests involving rollback on MySQL 4.1 should handle failures on non-transaction-supporting tables

Reported by: pb@… Owned by: nobody
Component: Database layer (models, ORM) Version:
Severity: minor Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


I've only tested this with MySQL 4.1.x. I don't know enough to debug it effectively at the moment, but wanted to make sure it got recorded.

Transaction support detection is tricky in MySQL, since the MySQL version and the table storage type are both factors. I'm also unsure what *should* happen in transaction tests with a DB that doesn't support transactions.

$ ./ 
Running tests with database 'mysql'

'transactions' module: API test failed
Code: 'Reporter.objects.all()'
Line: 38
Expected: '[Alice Smith, Ben Jones]\n'
Got: '[Alice Smith, Ben Jones, Carol Doe]\n'

'transactions' module: API test failed
Code: 'Reporter.objects.all()'
Line: 48
Expected: '[Ben Jones]\n'
Got: '[Ben Jones, Carol Doe]\n'

'transactions' module: API test failed
Code: 'Reporter.objects.all()'
Line: 59
Expected: '[Ben Jones, Carol Doe]\n'
Got: '[Carol Doe, Ben Jones, Carol Doe]\n'
3 errors:

("Tests" should be added as a Component in Trac, too, I think)

Change History (11)

comment:1 Changed 12 years ago by Jacob

Resolution: wontfix
Status: newclosed

I say this is an expected failure, and probably OK. Frankly, the fact that MySQL doesn't support transactions could be considered the bug, and thus this test behaves as expected :)

Unless someone can suggest a better way to deal with it I'm just going to leave this WONTFIX.

comment:2 Changed 12 years ago by pb@…

Resolution: wontfix
Status: closedreopened

Reopened to respectfully disagree.

I would suggest that, at a minimum, if you are going to say that Django officially supports MySQL (with no further qualifications in the docs about transaction support) then a working installation of MySQL should pass the test suite.

Otherwise I think that as the testing component becomes more widely used, you're just going to see variants of this ticket pop up over, and over, and over...

I know you don't like MySQL, but IMO you run a test suite to verify that there are no failures, not to get some failures and see whether they were "expected" or not.

Ultimately, of course, it's your call. If you feel strongly about transaction support perhaps the requirements in the docs should be revised to cover only MySQL setups that are transaction-capable.

comment:3 Changed 12 years ago by Adrian Holovaty

I was thinking we could do a special-case in the tests, as we do in the basic/ unit tests. Totally doable.

comment:4 Changed 12 years ago by mir@…

Just a note: I'm using mysql 4.1. does not report errors. That simply is because I'm using innodb as default engine. But you cannot force MySQL to use innodb, it might not be compiled in.

So, what?

comment:5 Changed 12 years ago by pb@…

Good question. Some options I can think of:

1) Have the special-case code (suggested by Adrian above) detect the storage engine MySQL is using (perhaps via a new flag in, "supports_transactions", analagous to the existing "supports_constraints"; offhand I don't know how to check the default storage engine without creating and inspecting a table, but I imagine there's a way)

2) Abandon all transaction tests for MySQL (that would be a shame)

3) Declare that only InnoDB tables are supported (and change Django's table generation code to explicitly request them, if it doesn't already)

comment:6 Changed 12 years ago by Luke Plant

Personally I agree with Jacob that this is a WONTFIX really - the tests are doing their job fine. But I realise it could be a bit friendlier for MySQL users...

A problem with using special casing to turn tests off is you need to know a lot about MySQL, including what backends are being used, possibly locating and inspecting a MySQL configuration file (whose format might change etc), plus you need to know the future. If you get it wrong, you end up excluding the tests when they ought to be run etc, which is quite a bad bug, since it allows other bugs to creep in undetected. Consider also that someone could easily have a custom build of MySQL with transactions backported, or accidentally broken or whatever.

I think perhaps a better solution would be to have some kind of custom error message you could print for a given module of tests e.g. transactions/ would have 'CUSTOM_ERROR_MESSAGE' which would be a callable whose output is appended to the error list when any tests in that module fail. It could detect MySQL from the settings and say "Transaction tests failed -- for MySQL, they are only supported for >= 5 with InnoDB backend." (or whatever other magic incantation is required).

comment:7 Changed 12 years ago by mir@…

I like lukeplant's solution. It's not dependant on the MySQL version, but on the storage engine. A proposal for a friendly text:

"Transaction test failed. Either they are really broken or you are using a default storage engine that does not support transactions. Consult your documentation to change the default storage engine to e.g. InnoDB"

(I think there are other engines which also support transactions.)

comment:8 Changed 12 years ago by pb@…

Sounds fine to me. And mir is correct, it looks like there are currently three storage engines with transaction support:

comment:9 Changed 11 years ago by Michael Radziej <mir@…>

Ehm, is this still open?

comment:10 Changed 11 years ago by Simon G. <dev@…>

Component: ToolsDatabase wrapper
Summary: Tests involving rollback fail on MySQL 4.1Tests involving rollback on MySQL 4.1 should handle failures on non-transaction-supporting tables
Triage Stage: UnreviewedAccepted

comment:11 Changed 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

MySQL hasn't given transaction-based errors in a long time for the tests. The normal tests only run under the default storage engine, since you cannot defer reference constraints until the end of a transaction in InnoDB, making fixture loading a problem. So, for all intents and purposes, this is fixed.

Note: See TracTickets for help on using tickets.
Back to Top