Fixed #4492 - Provide tests for mixed-case column names #1667

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@velis74
Contributor

velis74 commented Sep 23, 2013

https://code.djangoproject.com/ticket/4492
I created a model for testing mixed case entity names + a suite of 4 tests that test that. Since PostgreSQL lowercases everything not in quotes, usually only the last test would fail if this isn't implemented properly in the drivers.
Tested the unit-tests on PostgreSQL 9.2 and SQLite

@timgraham

View changes

tests/backends/models.py
@@ -110,3 +110,18 @@ class ObjectReference(models.Model):
def __str__(self):
return str(self.obj_id)
+
+@python_2_unicode_compatible
+class Mixed_Case_Table(models.Model):

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

I've never seen underscores in model names before, since I don't think we are testing that behavior, it may make sense to remove them. Also, please use double quotes for docstrings, I believe that's what the majority of Django uses.

@timgraham

timgraham Sep 23, 2013

Owner

I've never seen underscores in model names before, since I don't think we are testing that behavior, it may make sense to remove them. Also, please use double quotes for docstrings, I believe that's what the majority of Django uses.

This comment has been minimized.

Show comment Hide comment
@velis74

velis74 Sep 23, 2013

Contributor

I do apologize, but the conference ended and I don't know git well enough
to be able to commit changes that fast. I will not have any internet until
end of week so if the patch is otherwise OK, please have somebody perform
those minute changes.

Sorry about that,
Jure

On Mon, Sep 23, 2013 at 5:17 PM, Tim Graham notifications@github.comwrote:

In tests/backends/models.py:

@@ -110,3 +110,18 @@ class ObjectReference(models.Model):

 def __str__(self):
     return str(self.obj_id)

+@python_2_unicode_compatible
+class Mixed_Case_Table(models.Model):

I've never seen underscores in model names before, since I don't think we
are testing that behavior, it may make sense to remove them. Also, please
use double quotes for docstrings, I believe that's what the majority of
Django uses.


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/1667/files#r6517336
.

@velis74

velis74 Sep 23, 2013

Contributor

I do apologize, but the conference ended and I don't know git well enough
to be able to commit changes that fast. I will not have any internet until
end of week so if the patch is otherwise OK, please have somebody perform
those minute changes.

Sorry about that,
Jure

On Mon, Sep 23, 2013 at 5:17 PM, Tim Graham notifications@github.comwrote:

In tests/backends/models.py:

@@ -110,3 +110,18 @@ class ObjectReference(models.Model):

 def __str__(self):
     return str(self.obj_id)

+@python_2_unicode_compatible
+class Mixed_Case_Table(models.Model):

I've never seen underscores in model names before, since I don't think we
are testing that behavior, it may make sense to remove them. Also, please
use double quotes for docstrings, I believe that's what the majority of
Django uses.


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/1667/files#r6517336
.

@timgraham

View changes

tests/backends/models.py
+ Testing mixed case entity names, Pay attention to attribute names:
+ some characters are intentionally uppercase to see how your DB of choice handles them
+ '''
+ id = models.IntegerField(primary_key=True)

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

is there any reason to include an ID field explicitly?

@timgraham

timgraham Sep 23, 2013

Owner

is there any reason to include an ID field explicitly?

@timgraham

View changes

tests/backends/tests.py
+ but other RDBMSs should handle this test equally well
+ '''
+
+ def _build_records(self):

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

make this a setUp() method?

@timgraham

timgraham Sep 23, 2013

Owner

make this a setUp() method?

@timgraham

View changes

tests/backends/tests.py
+ '''
+
+ def _build_records(self):
+ rec = models.Mixed_Case_Table(id=1, sum_Field=2)

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

use Model.objects.create() rather than save()? I don't think manually specifying an id is needed (or a best practice)

@timgraham

timgraham Sep 23, 2013

Owner

use Model.objects.create() rather than save()? I don't think manually specifying an id is needed (or a best practice)

@timgraham

View changes

tests/backends/tests.py
@@ -964,6 +964,41 @@ def equal(value, max_d, places, result):
equal('0.1234567890', 12, 0,
'0')
+class TestMixedCaseEntityNames(TestCase):
+ '''
+ Test for mixed case entity names. Will test particulary PostgreSQL,

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

Start the docstring with "#4492 --" to reference the ticket

@timgraham

timgraham Sep 23, 2013

Owner

Start the docstring with "#4492 --" to reference the ticket

@@ -11,7 +11,7 @@
from django.conf import settings
from django.core.management.color import no_style
from django.db import (connection, connections, DEFAULT_DB_ALIAS,
- DatabaseError, IntegrityError, transaction)
+ DatabaseError, IntegrityError, transaction, migrations)

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

migrations isn't used

@timgraham

timgraham Sep 23, 2013

Owner

migrations isn't used

@timgraham

View changes

tests/backends/tests.py
+
+ def test_mixed_case_table_create(self):
+ # Note that the table is already created, so we just insert the records
+ statements = connection.creation.sql_create_model(models.Mixed_Case_Table,

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

I'm unclear with the purpose of this line is -- the "statements" local variable isn't used

@timgraham

timgraham Sep 23, 2013

Owner

I'm unclear with the purpose of this line is -- the "statements" local variable isn't used

@@ -110,3 +110,18 @@ class ObjectReference(models.Model):
def __str__(self):
return str(self.obj_id)
+

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

two spaces between class names please

@timgraham

timgraham Sep 23, 2013

Owner

two spaces between class names please

@timgraham

View changes

tests/backends/tests.py
+ self.assertEqual(res.values()[0], 5)
+
+ def test_mixed_case_table_select_manual(self):
+ '''Manual check because if nothing is quoted, everything works, otherwise, it all fails'''

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

I'm not sure what this sentence means

@timgraham

timgraham Sep 23, 2013

Owner

I'm not sure what this sentence means

@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Sep 23, 2013

Owner

As noted in the ticket, I think we have tests that would cover a regressions for the ticket in question, so I'm going to close this. Sorry for wasting your time in that respect, but I hope you learned the basics of contributing to Django and will help out in the future.

Owner

timgraham commented Sep 23, 2013

As noted in the ticket, I think we have tests that would cover a regressions for the ticket in question, so I'm going to close this. Sorry for wasting your time in that respect, but I hope you learned the basics of contributing to Django and will help out in the future.

@timgraham timgraham closed this Sep 23, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment