Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #14415 -- Do not ignore DB test name setting #6536

Closed
wants to merge 3 commits into from

Conversation

boaz85
Copy link
Contributor

@boaz85 boaz85 commented Apr 29, 2016

Modified the code so it will take DATABASES['db-name']['TEST']['NAME'] into account while it decides which databases to create for tests. Using this setting is essential when testing with different tables in different databases

Modified the code so it will take DATABASES['db-name']['TEST']['NAME'] into account while it decides which databases to create for tests. Using this setting is essential when testing with different tables in different databases
@timgraham
Copy link
Member

Did you consider trying to write a test for this change? If not, did you try testing it manually?

@boaz85
Copy link
Contributor Author

boaz85 commented May 7, 2016

I did consider, but couldn't think of a standard way to do this.
I tested it in two projects:

  1. A real project where I actually faced this issue.
  2. A clean test project with this setup:
  • Two DBs in the DATABASES setting. The only difference between them is that the non-default one has ['TEST']['NAME'] setting.
  • One django app that contains two models.
  • A database router that redirects each model to be handled on a different db (db_for_write and allow_migrate were overridden to achieve this).
  • One test case that only creates one row in each model table.

Before the fix I got
ProgrammingError: relation "some_relation" does not exist

Because Django didn't find the second model table on the first DB. It wan't created because it shouldn't be created on the default db, but the second db was not created because Django assumed it is already exists (after ignoring the ['TEST']['NAME'] setting).

After the fix the test has passed.

@shaib
Copy link
Member

shaib commented May 7, 2016

@boaz85 three points:

  • I agree that there is no good way to add a test for the fix.
  • when no test-name is defined, you should use the default test-name in the signature rather than the production name. This is required to make behavior consistent when one of the databases has the test name specified and the other doesn't.
  • Please review the relevant documentation and see if anything needs to be changed. If so, it may be also useful to add a note in the release notes.

Thanks for working on this!

@boaz85
Copy link
Contributor Author

boaz85 commented May 12, 2016

I changed the code to use get_test_db_name to get the db name for the signature. It behaves exactly like Shai suggested and seems to be cleaner.

I didn't find any doc that should be updated. After all - this is a bug fix and not a new feature, right? Should I update the latest version (1.9.6) release file?

@timgraham
Copy link
Member

I think a test something like this might be feasible:

from django.db.backends.base.creation import BaseDatabaseCreation
from django.db import connection
import copy
connection = copy.copy(connection)
connection.settings_dict = {..}
self.assertEqual(BaseDatabaseCreation(connection).test_db_signature(), ....)

You might be able to do this more elegantly if you look at tests/backends/tests.py for other strategies.

This isn't a critical fix or a regression from earlier versions of Django, so it won't be backported to 1.9.

@boaz85 boaz85 force-pushed the ticket_14415 branch 3 times, most recently from c511da2 to c5edb12 Compare May 13, 2016 23:45
@boaz85
Copy link
Contributor Author

boaz85 commented May 17, 2016

Added regression tests. They all fail without the fix and pass with it.

@timgraham
Copy link
Member

merged in 5f23f90, thanks!

@timgraham timgraham closed this May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants