Opened 6 years ago

Last modified 2 years ago

#11665 new Bug

django.test.TestCase should flush constraints

Reported by: Glenn Owned by:
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: glenn@…, peruano@…, aball@…, forest, nperriault@…, jim.dalton@…, graham@…, botondus@…, vlastimil.zima@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In Postgresql, constraints are set DEFERRABLE INITIALLY DEFERRED. This causes it to only check those constraints when the transaction is committed. (This may affect the Oracle backend, too, based on a quick search.)

This causes a problem with TestCase: as they're never committed, many constraint violations can never be triggered.

It's an easy fix: cause SET CONSTRAINTS ALL IMMEDIATE to be executed in TestCase before rolling back the transaction, so any deferred constraints are flushed, and treat an exception from that as a test failure.

I'll work on a patch in a week or so when I have some time. I'm not sure how to test this, since it means testing the testing system itself.

Attachments (8)

11665.1.diff (2.4 KB) - added by ramiro 4 years ago.
Initial patch - It causes six ERRORs in the multiple_database regression tests
11665.2.diff (5.0 KB) - added by ramiro 4 years ago.
Better patch (use addError() instead of reraising exceptions in _fixture_teardown()) + workaround for one of the failing multiple_database tests it introduces
better_constraint_checks_during_testing.v1.diff (32.9 KB) - added by jsdalton 4 years ago.
better_constraint_checks_during_testing.v2.diff (38.6 KB) - added by jsdalton 4 years ago.
better_constraint_checks_during_testing.v3.diff (40.3 KB) - added by jsdalton 4 years ago.
better_constraint_checks_during_testing.v4.diff (21.3 KB) - added by jsdalton 4 years ago.
better_constraint_checks_during_testing.v5.diff (19.5 KB) - added by jsdalton 4 years ago.
better_constraint_checks_during_testing.v6.diff (19.6 KB) - added by jsdalton 4 years ago.

Download all attachments as: .zip

Change History (51)

comment:1 Changed 6 years ago by Glenn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

This isn't a silent failure; the test runner fails later on when it tries to flush the database--the TRUNCATE command gives "cannot TRUNCATE "table" because it has pending trigger events", where the trigger events are actually the constraints failing. SET CONSTRAINTS ALL IMMEDIATE will give it a correct error message, showing which constraint is actually being violated.

comment:2 Changed 6 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 6 years ago by pcardune

Any idea when a patch will become available? I just encountered this same problem.

comment:4 Changed 6 years ago by russellm

@pcardune: The patch will become available about 30 seconds after somebody writes it.

The solution here isn't to change the constraint definition. Constraints need to be DEFERRABLE, not IMMEDIATE - there are a number of tests that fail if constraint checks aren't deferred. To see which ones, run the test suite using MySQL with InnoDB. InnoDB only has IMMEDIATE mode for transactions, and fails a lot of tests as a result.

The interim workaround - if you actually need transactions, use TransactionTestCase rather than TestCase. This is slower, but uses a database truncate rather than transaction rollback to flush the database.

I don't know what the final solution will be, but I suspect it will involve reliable cleanup of the transaction on rollback, rather than changing the database so that the triggers aren't hit.

comment:5 Changed 6 years ago by Glenn

Nobody's suggested changing the constraints; I suspect you misunderstand what SET CONSTRAINTS ALL IMMEDIATE does. See
http://www.postgresql.org/docs/current/static/sql-set-constraints.html. Execute this at the end of a test case, just before it rolls back, so it executes the constraint checks as if the transaction was committed.

I've been busy, but I'll try to get to this eventually if nobody gets to it first.

comment:6 Changed 6 years ago by russellm

Apologies - I misunderstood your proposal. Chalk that one up to my morning coffee not kicking in yet. Now that I'm actually reading what you wrote, your proposal looks to be just what is required.

comment:7 Changed 5 years ago by olaechea

  • Owner changed from nobody to olaechea
  • Status changed from new to assigned

comment:8 Changed 5 years ago by olaechea

  • Owner olaechea deleted
  • Status changed from assigned to new

comment:9 Changed 5 years ago by olaechea

Fixing this would involve changing the testcase class and the database drivers for each of the supported databases ?

comment:10 Changed 5 years ago by olaechea

  • Cc peruano@… added

comment:11 Changed 5 years ago by anonymous

  • Cc aball@… added

comment:12 Changed 5 years ago by forest

  • Cc forest@… added

comment:13 Changed 5 years ago by bretthoerner

  • Cc brett@… added

comment:14 Changed 5 years ago by ubernostrum

  • milestone 1.2 deleted

1.2 is feature-frozen, moving this feature request off the milestone.

comment:15 Changed 5 years ago by forest

  • Cc forest added; forest@… removed

comment:16 Changed 4 years ago by n1k0

  • Cc nperriault@… added

comment:17 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

Changed 4 years ago by ramiro

Initial patch - It causes six ERRORs in the multiple_database regression tests

comment:18 Changed 4 years ago by ramiro

  • Easy pickings unset
  • Has patch set
  • Patch needs improvement set
  • UI/UX unset

I've attached my intepretation of how this could be implemented.

There is still some issue (either in the multiple_database tests or the ORM functionality they exercise) because applying this patch make six errors to appear in the full test suite, all of them in these tests. All of this when running against Postgres, of course, it shoudn't affect other backends.

Also, IMHOwe can make an exception regarding tests for this particular issue.

comment:19 Changed 4 years ago by ramiro

Forgot to paste the errors:

======================================================================
ERROR: test_generic_key_reverse_operations (regressiontests.multiple_database.tests.QueryTestCase)
Generic reverse manipulations are all constrained to a single DB
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/test/testcases.py", line 296, in __call__
    self._post_teardown()
  File "django/test/testcases.py", line 312, in _post_teardown
    self._fixture_teardown()
  File "django/test/testcases.py", line 604, in _fixture_teardown
    connections[db].check_constraints()
  File "django/db/backends/__init__.py", line 261, in check_constraints
    self.cursor().execute(s)
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
IntegrityError: insert or update on table "multiple_database_review" violates foreign key constraint "multiple_database_review_content_type_id_fkey"
DETAIL:  Key (content_type_id)=(1) is not present in table "django_content_type".


======================================================================
ERROR: test_generic_key_separation (regressiontests.multiple_database.tests.QueryTestCase)
Generic fields are constrained to a single database
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/test/testcases.py", line 296, in __call__
    self._post_teardown()
  File "django/test/testcases.py", line 312, in _post_teardown
    self._fixture_teardown()
  File "django/test/testcases.py", line 604, in _fixture_teardown
    connections[db].check_constraints()
  File "django/db/backends/__init__.py", line 261, in check_constraints
    self.cursor().execute(s)
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
IntegrityError: insert or update on table "multiple_database_review" violates foreign key constraint "multiple_database_review_content_type_id_fkey"
DETAIL:  Key (content_type_id)=(1) is not present in table "django_content_type".


======================================================================
ERROR: test_foreign_key_cross_database_protection (regressiontests.multiple_database.tests.RouterTestCase)
Foreign keys can cross databases if they two databases have a common source
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/test/testcases.py", line 296, in __call__
    self._post_teardown()
  File "django/test/testcases.py", line 312, in _post_teardown
    self._fixture_teardown()
  File "django/test/testcases.py", line 604, in _fixture_teardown
    connections[db].check_constraints()
  File "django/db/backends/__init__.py", line 261, in check_constraints
    self.cursor().execute(s)
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
IntegrityError: insert or update on table "multiple_database_book" violates foreign key constraint "multiple_database_book_editor_id_fkey"
DETAIL:  Key (editor_id)=(20) is not present in table "multiple_database_person".


======================================================================
ERROR: test_generic_key_cross_database_protection (regressiontests.multiple_database.tests.RouterTestCase)
Generic Key operations can span databases if they share a source
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/test/testcases.py", line 296, in __call__
    self._post_teardown()
  File "django/test/testcases.py", line 312, in _post_teardown
    self._fixture_teardown()
  File "django/test/testcases.py", line 604, in _fixture_teardown
    connections[db].check_constraints()
  File "django/db/backends/__init__.py", line 261, in check_constraints
    self.cursor().execute(s)
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
IntegrityError: insert or update on table "multiple_database_review" violates foreign key constraint "multiple_database_review_content_type_id_fkey"
DETAIL:  Key (content_type_id)=(1) is not present in table "django_content_type".


======================================================================
ERROR: test_generic_key_managers (regressiontests.multiple_database.tests.RouterTestCase)
Generic key relations are represented by managers, and can be controlled like managers
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/test/testcases.py", line 296, in __call__
    self._post_teardown()
  File "django/test/testcases.py", line 312, in _post_teardown
    self._fixture_teardown()
  File "django/test/testcases.py", line 604, in _fixture_teardown
    connections[db].check_constraints()
  File "django/db/backends/__init__.py", line 261, in check_constraints
    self.cursor().execute(s)
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
IntegrityError: insert or update on table "multiple_database_review" violates foreign key constraint "multiple_database_review_content_type_id_fkey"
DETAIL:  Key (content_type_id)=(1) is not present in table "django_content_type".


======================================================================
ERROR: test_m2m_managers (regressiontests.multiple_database.tests.RouterTestCase)
M2M relations are represented by managers, and can be controlled like managers
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/test/testcases.py", line 296, in __call__
    self._post_teardown()
  File "django/test/testcases.py", line 312, in _post_teardown
    self._fixture_teardown()
  File "django/test/testcases.py", line 604, in _fixture_teardown
    connections[db].check_constraints()
  File "django/db/backends/__init__.py", line 261, in check_constraints
    self.cursor().execute(s)
  File "django/db/backends/postgresql_psycopg2/base.py", line 44, in execute
    return self.cursor.execute(query, args)
IntegrityError: insert or update on table "multiple_database_book_authors" violates foreign key constraint "multiple_database_book_authors_person_id_fkey"
DETAIL:  Key (person_id)=(1) is not present in table "multiple_database_person".


----------------------------------------------------------------------
Ran 3861 tests in 2135.680s

FAILED (errors=6, skipped=50, expected failures=3)

Changed 4 years ago by ramiro

Better patch (use addError() instead of reraising exceptions in _fixture_teardown()) + workaround for one of the failing multiple_database tests it introduces

comment:20 Changed 4 years ago by jsdalton

I've been doing some work on #3615 recently that I believe touches upon (and hopefully resolves?) this issue. That ticket refers to a problem in MySQL under InnoDB, where fixtures with forward references fail to load. The solution is to disable foreign key checks in MySQL, which then creates a situation where you need to immediately check constraints after data has been loaded. This is done using a special SQL SELECT statement for MySQL and sqlite. The most recent patch (uploaded last night) is designed to work with all backends, i.e. it adds a framework any backend can implement to check constraints immediately.

The primary difference between the patch there and this one is that the check_constraints call happens in my patch right after the data is loaded, rather than at the end of the test run. It's actually implemented in the loaddata command, so even more importantly it will check data being loaded via that command or initial data loaded via syncdb.

Anyhow, it should be as simple as overriding BaseDatabaseWrapper.check_constraints() in postgresql with an implementation that executes that SET CONSTRAINTS ALL IMMEDIATE statement. Note that check_constraints takes a list of tables as a kwarg and will be called with this, but postgresql doesn't need to use it. This method will be called after the data is loaded for every fixture. If for some reason that presents a problem, the alternative would be to implement get_key_columns() on postgresql's Introspection class. The default check_constraints() will check all of the key columns in a table for foreign key violations via ordinary SQL that should be platform independent. Doubt you guys would need to do this, but if for some reason it's an issue to run the set constraints immediate on every load that's an alternative to explore.

comment:21 Changed 4 years ago by jsdalton

  • Cc jim.dalton@… added

comment:22 Changed 4 years ago by graham_king

  • Cc graham@… added

comment:23 Changed 4 years ago by jsdalton

I think the correct solution here is to enable constraint checks at the *beginning* of the test transaction, for all backends. They can then be temporarily deferred as needed, e.g. during fixture loading or object deletion.

I have done this as part of an experimental branch I was working on for #3615, diff here: http://paste.pocoo.org/show/437952/ and not only does it work fine, it revealed a whole bunch of errors that have silently accumulated in the test suite. In fact, the reason the MySQL InnoDB test suite has been failing so long is because InnoDB's execution model during tests is closer to production, and MySQL was in fact revealing these real errors.

In working through these, I have already managed to eliminate them and get them down to just a handful, for all backends. All that's required now is working through the handful of remaining bugs and perhaps bolstering things with a few additional tests.

Before I proceed I'd like to know if there's any consensus on the idea I'm presenting here, or whether anybody has any objections. I posted this to the dev list and didn't get a ton of feedback (https://groups.google.com/d/topic/django-developers/J_5SuL0MoQU/discussion). Does anybody watching this ticket have any thoughts? If you think I'm wrong I'd like to hear it, so I can either reconsider my view or answer any objections you might have.

Thanks for listening.

comment:24 Changed 4 years ago by aaugustin

#16457 was a duplicate.

comment:25 Changed 4 years ago by jsdalton

Attached is an initial patch which encapsulates a slightly modified approach to the one outlined in my previous comment and originally proposed on the dev list.

In short, Simon Riggs pointed out in that discussion (https://groups.google.com/d/topic/django-developers/J_5SuL0MoQU/discussion) that my conception of how transactions work was too naive and my proposed solution (to set constraint checks immediately at the start) did not in fact accurately reflect how things work in production.

Fortunately, the solution wasn't too hard. Emulating production behavior better is a simple matter of adjusting the way we monkey patch the transaction functions in the set up of TestCase. Rather than simply stubbing those out, I added the constraint checking feature from #3615 in its place. So even though the main transaction functions don't *do* what they do in production (i.e. actually commit or rollback the function), they now perform the constraint check they normally would at that point and raise the appropriate IntegrityError when they do.).

The patch also includes a test which demonstrates that an IntegrityError should be raised during a TestCase test if bad data is entered.

The patch is not complete for several reasons:

  • Haven't addressed any of the errors that this raises in the rest of the suite yet. I have some solutions for many but I'd rather focus on getting a solution to the immediate issue down.
  • I have one concern about the implementation of check_constraints() on postgresql (and Oracle perhaps). If the autocommit feature is on, then does it make sense to revert to deferred? I think those statements are all irrelevant if that mode is on, so I think the answer is probably no.
  • I wonder if it makes sense to make use of a savepoint at the start of tests (and release it at the end), and then rollback to that savepoint if for any reason a constraint check fails? That would allow tests to continue even if a (planned) IntegrityError was hit. This seems like a rather obscure feature, but there is a theoretical possibility of tests that previously worked (when we never checked constraints) which might fail now, since once you hit an IntegrityError the test is for all intents and purposes over.

Note that this patch includes the entire latest patch from #3615, because it depends on that patch in order to work. Here's a diff between this patch and that patch which highlights the focus of this patch (http://paste.pocoo.org/show/440770/).

comment:26 Changed 4 years ago by jsdalton

Here's an improved patch that gets us closer. Basically this one starts to fix the errors that come up once you enable production-like constraint checking. You can see the commits here: https://github.com/jsdalton/django/commits/better_constraint_checks_during_testing but in a nutshell:

  • I reworked the check_constraints() feature (from #3615) a bit to ensure we *don't* check constraints if the constraint_checking_disabled context manager is in effect. This prevented a regression that occurred in fixture loading when the new check_constraints() function I added to django.test.testcases got executed.
  • Flush the ContentType cache during tests, which cleared up some issues in egression_tests.multiple_database. Also fixed an assertNumQueries count after the change, since a new contenttype select was being executed.
  • Add @ignore_num_queries decorator. Basically, because the check_constraints() implementation can have a varying number of SQL statements depending on the backend, I implemented a decorator that prevents assertNumQueries from counting queries on any statements executed during a decorated function. For now just added it to django.test.testcases.check_constraints().

Any comments on the above? This work starts to deviate a bit from this ticket, but it's required to get tests passing.

This gets us to 3 tests away from passing all tests in Postgresql, with correct constraint checking. The only 3 tests we're missing now in multiple_database. I know ramiro had done some work on some of these. I guess I'll sit down with them at some point, but I'm kind of a bit stuck... If anyone has thoughts I'd love to hear them.

Next up for me is hitting the remaining failiures on MySQL. Almost there...

comment:27 Changed 4 years ago by jsdalton

  • Patch needs improvement unset

Okay, did a bit more work on this and I think we're there -- for this ticket at least.

Basically after running tests last night and analyzing the results, there are *only* two problems that exist:

  • A few multiple_database tests throw errors (the ones ramiro already identified). In this patch, I have simply disabled constraint checking for those test cases (in setUp and tearDown). This puts the tests back where they are now -- since right now there are no constraint checks being run and that's why they are passing. I propose we open a new, separate ticket for the issue because frankly I think it's out of the scope of this ticket to make a decision about how it should be handled.
  • A few select_for_update errors in MySQL. Basically, two of the select_for_update tests are failing for MySQL. They are failing presently (without the patch) and I don't have an easy solution for them either. Again, I propose we open a new ticket to handle these. It appears they are legitimate bugs from what I can tell -- the code being exercised is not behaving as expected, and this needs to be addressed.

Removing the patch needs improvement flag, because this patch should be ready for checkin -- assuming no one has any concerns with the approach I took to solve the various little issues. Also, do we need documentation?

Note again that that *includes* the patch from #3615 because that has not been committed yet, and the code there is required for this. For convenience, here again is a diff against that other patch which highlights more clearly the actual work being done for this ticket: http://paste.pocoo.org/show/442244/

comment:28 Changed 4 years ago by jsdalton

Just opened #16490, a report on the select_for_update error I mentioned above. Once those two failures are fixed, all backends will pass all tests.

comment:29 Changed 4 years ago by graham_king

The v3 patch applies cleanly on latest SVN (16549) with patch -p1, and, to my very happy surprise, all the tests pass with MySQL / InnoDB! I'm going to wipe my Django checkout and try again with everything clean just to be 100% sure.

On MySQL 5.1.54 / InnoDB I get: Ran 4030 tests in 9 hours, OK (skipped=58, expected failures=3).

For comparison in SQLite I got: Ran 4021 tests in 5 minutes, OK (skipped=64, expected failures=3).

Unless I've messed something up with my environment, I looks like this combined patch marks a historic moment in Django's history. Thanks!

comment:30 Changed 4 years ago by russellm

See #3615 for some comments on the constraint-checking part of this patch.

comment:31 Changed 4 years ago by jsdalton

@graham_king - That's great to hear :) You did one better than me; I still have two failing tests in MySQL (unrelated to this ticket), which I filed a ticket for. Hopefully it goes well again when you do a clean wipe.

Also, I replied to Russell's concern on #3615. The short answer is we should be okay as is. I still might try to track down Simon Riggs, who was quite helpful with some other Postgresql questions on the developers list conversation, to be sure.

comment:32 Changed 4 years ago by graham_king

With a clean checkout from SVN, and this patch, same result: all the tests pass on MySQL InnoDB. Fantastic!

comment:33 Changed 4 years ago by jsdalton

Now that #3615 is committed this patch stopped applying cleanly. I've cleaned it up and it should apply to trunk just fine.

Bad news for me is that I'm back to getting failures again in MySQL. I get 3 failures and 1 error when I run the whole suite, but only 1 failure when I run each failing test in isolation. Postgres has no failures or errors though.

None of the MySQL errors are related to this patch or ticket, so it should be ready for checkin.

comment:34 Changed 4 years ago by jsdalton

Quick clarification to my comment above. The errors I came across in MySQL are not related to this patch or ticket, and they all have tickets opened for them.

Here's the test output:

$ ./runtests.py --settings=testproject.settings
Creating test database for alias 'default'...
Creating test database for alias 'other'...
.......................................................................................................................................................................................................................................................................................................................................................................................................s...............s..................................................................s...s...............................................................s.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................s........s.sss................................................................................................................................................................x...........................................................................................................................s.........................................................................................................................................x.........................................................................................................................s.......................................................................................................................................................................................................................................................................................................s...........................................................................s.............x...........................................................................................................................................................................................................................................................s.......................................................................................................................................................................................................................................................................................................................................sEF.ss.............s.......s..............................................................................................................................................................................................................................................................................................................................ssss........F......................................................................................................................................................................................................................................ss......................s.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................F................................................................................................................................................................ss..s....
======================================================================
ERROR: test_table_exists (modeltests.proxy_model_inheritance.tests.ProxyModelInheritanceTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jsdalton/webs/testproject/src/django/tests/modeltests/proxy_model_inheritance/tests.py", line 25, in setUp
    call_command('syncdb', verbosity=0)
  File "/Users/jsdalton/webs/testproject/src/django/django/core/management/__init__.py", line 166, in call_command
    return klass.execute(*args, **defaults)
  File "/Users/jsdalton/webs/testproject/src/django/django/core/management/base.py", line 220, in execute
    output = self.handle(*args, **options)
  File "/Users/jsdalton/webs/testproject/src/django/django/core/management/base.py", line 351, in handle
    return self.handle_noargs(**options)
  File "/Users/jsdalton/webs/testproject/src/django/django/core/management/commands/syncdb.py", line 121, in handle_noargs
    custom_sql = custom_sql_for_model(model, self.style, connection)
  File "/Users/jsdalton/webs/testproject/src/django/django/core/management/sql.py", line 150, in custom_sql_for_model
    app_dir = os.path.normpath(os.path.join(os.path.dirname(models.get_app(model._meta.app_label).__file__), 'sql'))
  File "/Users/jsdalton/webs/testproject/src/django/django/db/models/loading.py", line 151, in get_app
    raise ImproperlyConfigured("App with label %s could not be found" % app_label)
ImproperlyConfigured: App with label unmanaged_models could not be found

======================================================================
FAIL: test_block (modeltests.select_for_update.tests.SelectForUpdateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jsdalton/webs/testproject/src/django/django/test/testcases.py", line 645, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/Users/jsdalton/webs/testproject/src/django/django/test/testcases.py", line 645, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/Users/jsdalton/webs/testproject/src/django/tests/modeltests/select_for_update/tests.py", line 209, in test_block
    self.assertEqual('Fred', p.name)
AssertionError: 'Fred' != u'Reinhardt'

======================================================================
FAIL: test_bug_8245 (regressiontests.bug8245.tests.Bug8245Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jsdalton/webs/testproject/src/django/tests/regressiontests/bug8245/tests.py", line 18, in test_bug_8245
    'autodiscover should have raised a "Bad admin module" error.')
AssertionError: autodiscover should have raised a "Bad admin module" error.

======================================================================
FAIL: test_load_error (regressiontests.templates.tests.TemplateTagLoading)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jsdalton/webs/testproject/src/django/tests/regressiontests/templates/tests.py", line 1636, in test_load_error
    self.assertTrue('ImportError' in e.args[0])
AssertionError: False is not True

----------------------------------------------------------------------
Ran 4029 tests in 14788.690s

FAILED (failures=3, errors=1, skipped=30, expected failures=3)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

And here are the tickets which refer to each of these:

  • #16592 is behind the test_table_exists error
  • #13906 is behind the test_block failure
  • #16593 is behind the test_bug_8245 and test_load_error failures. These are just occurring because the error in test_table_exists is causing some stuff not to get restored properly.

comment:35 Changed 4 years ago by jsdalton

Patch wasn't applying cleanly so I fixed it up a bit.

With this patch, I'm currently one test error (#16592) away from all tests passing on MySQL.

comment:36 Changed 4 years ago by bberes

  • Cc botondus@… added

comment:37 Changed 4 years ago by jsdalton

Patch wasn't applying cleanly again.

comment:38 Changed 3 years ago by jsdalton

Pasting a conversation between myself and jacobkm on IRC:

[4:50pm] jacobkm: jsdalton: what's connections._ignore_num_queries for?

[4:50pm] • jacobkm smells a hack

[4:50pm] jsdalton: jacobkm: lol. the problem is that MySQL uses a different number of queries to perform certain operations, basically it's inconsistent

[4:51pm] jsdalton: mostly this had to do with fixture loading, if i recall correctly

[4:51pm] dgovinda left the chat room. (Ping timeout: 276 seconds)

[4:51pm] dgovinda joined the chat room.

[4:51pm] jsdalton: to be honest, expecting a consistent query count across multiple backends with differing implementations sort of struck me as problematic

[4:52pm] jacobkm: jsdalton: so it's a workaround to make assertNumQueriese consistent across backends?

[4:52pm] jacobkm: jsdalton: I'd rather see something more like "if backend == mysql: assertNumQuerys(6)" sort of thing.

[4:52pm] jsdalton: hm

[4:52pm] jacobkm: This seems... evil and undocumented.

[4:52pm] jsdalton: hm, thinking

[4:53pm] jacobkm: IOW I'd rather work around it in the tests than have it all up in django.db.

[4:54pm] jacobkm: jsdalton: other than that I think this looks really good. We'll need to put something in the release notes -- this has potential backwards-compatibility implications -- but if you've not got time I can take care of that.

[4:55pm] jsdalton: jacobkm: cool, sorry just opening up my code here and reuploading my thought process to my brain

[4:55pm] jacobkm: jsdalton: no worries!

[4:56pm] jacobkm: jsdalton: I'm headed for lucnch but I'll check in when I get back so leave anything here for me if you like.

[4:56pm] jsdalton: sounds good i'll see if i can recall my train of thought by then

[5:00pm] jsdalton: jacobkm: for when you return, here's a very brief summary of my reasoning behind ignore_num_queries for you to consider

[5:02pm] jsdalton: first, the reason for is the implementation of check_constraints() varies across backends (i believe MySQL and SQLite are different from the others)

[5:03pm] jsdalton: so yeah one solution would be to set up different assertions for each backend in those tests where they were inconsistent. that was probably what i wanted to do but i felt like that was cheating, since that could conceivably be prone to breakage down the road as implementation details changed

[5:03pm] jsdalton: be that as it may, i'd probably be fine with that

[5:05pm] jsdalton: i implemented ignore_num_queries because it seemed like a better more flexible long term solution. i did add tests for it as well, btw.

[5:05pm] jsdalton: if you look closely at the implementation, it doesn't *really* touch production code

[5:06pm] jsdalton: it adds a single private attribute to the connections variable in django.db

[5:07pm] jsdalton: otherwise, the only other place it lives is in CursorDebugWrapper, i.e. the same place where num queries does its thing

[5:09pm] jsdalton: and for now it's only used in the test suite around a piece of test code that emulates constraint checking

[5:10pm] jsdalton: anyhow, not sure this is a very strong case for it. to me it felt like less of a hack because it's just a way of telling assert num queries to ignore query counts for certain blocks of code, but if my argument is not particularly convincing i can take your suggestion of specifying different query counts per backend

If we can get a decision on @ignore_num_queries as a decorator vs. specifying different expected query counts per backend, I'd be happy to implement either and proceed. Jacob also mentioned documentation in the release notes, which I can try to tackle as well.

comment:39 Changed 3 years ago by aaugustin

On IRC, Alex and I confirmed that it would be better to avoid the _ignore_num_queries hack and just make the test database-dependent instead.

comment:40 Changed 3 years ago by bretthoerner

  • Cc brett@… removed

comment:41 Changed 3 years ago by jsdalton

There ended up being a lot of work in this ticket that related more to fixing errors in the MySQL test suite than the original issue here, which is whether and how constraint checking should be implemented during testing.

A few things:

  1. I created a new ticket (#17055) to deal with the MySQL related issues separately.
  1. While I"m still convinced of the merits of what I ultimately attempted to do (implement constraint checks at transaction commit points to better emulate production behavior and ensure the integrity of data in the test databases at all times during testing), the implementation is not in a good place for constraint checking in MySQL and SQLite. When running the test suites today I realized that triggering constraint checks at every transaction commit point creates unacceptable slowness in the test suite for those backends. It runs fine on PostgreSQL however. I do not have a good idea for how to work around it at this point in time. Also, the constraint checking creates a variable number of SQL queries making the suggestions for alternatives to my ignore_num_queries approach not quite as tenable. More thinking needs to go into that.

In short, it's hard for me to see a very clear path forward for this ticket. Checking for constraints via SQL statements is slow and there's not a great way to work around it.

The one idea I have is we could do a "supports_fast_constraint_checks" feature and only do the check constraints on commit if a backend had that. The benefit here is we would catch data integrity errors before they arise in PostgreSQL but wouldn't cause horrible slowdowns in MySQL and SQLite. We could then also avoid the different query counts issues, or at least apply one of those suggestions.

Anyhow, if anyone has any comments on this let me know. I'm having a hard time motivating myself to push through a patch like that unless there's interest.

Cheers.

comment:42 Changed 3 years ago by lrekucki

  • Patch needs improvement set

comment:43 Changed 2 years ago by vzima

  • Cc vlastimil.zima@… added
Note: See TracTickets for help on using tickets.
Back to Top