Django should load custom SQL when testing

[Danilo Bargen]

As the Ticket https://code.djangoproject.com/ticket/16550 is closed, I will 
continue the discussion here on the mailing list.

The reason for the ticket is that the Django test runner does not provide a 
way to run custom SQL. This makes sense when users want to load custom SQL 
*data*, but not when modifying the way the database works.

Here's my comment from the issue comment thread:

I agree very much with the comment above, in that there should be a way to 
execute custom SQL before and after running syncdb. This should not be used 
to load custom data, which is truncated anyways, but to alter the schema, 
create new types or load extensions. 

In my case, I have to load the Postgres citext extension:  

CREATE EXTENSION IF NOT EXISTS citext;

Because I have to create a database manually before running syncdb, I can 
run my custom SQL in my database and then use it normally with Django. But 
because the extensions are database specific, when running the tests (which 
create their own database) they fail because the extension is not present. 
This means that contrary to the comment by russellm, the *test database* 
introduces an inconsistency between the way tables work during a test case 
and the way they work in production. 

Another widely used extension is the hstore extension. 

As a workaround, I loaded the extension in the default postgres template. 

psql -d template1 -c 'CREATE EXTENSION citext;' 

But that means that any new database I create will contain the citext 
extension, which might not be what I want.
Therefore I suggest that you add some pre syncdb and post syncdb hooks to 
run custom SQL, and to mention in the docs that this should *not* be used 
to load custom data, but to create new types and alter the schema, so that 
it *matches the production database*. The data is truncated anyways. 

I vote for reopen.

What do you think about this issue? Does anyone else agree that there 
should be a way to run custom SQL in order not to create inconsistencies 
between the test and production database?

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To view this discussion on the web visit 
https://groups.google.com/d/msg/django-developers/-/KsrsEf7XOA8J.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Django TestCase isolation: setUp vs setUpClass

[Danilo Bargen]

Hi all

Today I ran across an issue while debugging Django tests. As stated in the 
docs, all model changes in a TestCase that are done during the test are 
reverted between the tests. This creates a nice and useful isolation of the 
different tests. Each time a test runs, the database looks just the way it 
was after the syncdb.

> from django.test import TestCase
> from apps.front import models 
> 
> class Test1(TestCase): 
> def setUp(self):
> models.User.objects.create(username='spamham') 
> 
> def test(self):
> pass 
> 
> class Test2(TestCase):
> def setUp(self):
> models.User.objects.create(username='spamham') 
> 
> def test(self):
> pass

These test run fine, even though two users with the same PK are created, 
because Django handles test isolation. But things behave differently when 
using the setUpClass classmethod. I used setUpClass instead of setUp 
because there are certain models that I just want to create once for all 
the tests in a test case. It would be an unnecessary performance loss if 
those model instances were created and then rolled back for each test 
method. And less DRY.

The problem is, model changes that are done in setUpClass aren't rolled 
back between the test cases.

> from django.test import TestCase
> from apps.front import models
> 
> class Test1(TestCase):
> @classmethod
> def setUpClass(cls):
> models.User.objects.create(username='spamham')
> 
> def test(self):
> pass
> 
> class Test2(TestCase):
> @classmethod
> def setUpClass(cls):
> models.User.objects.create(username='spamham')
> 
> def test(self):
> pass

This fails with an IntegrityError due to a violation of 
the front_user_username_key unique constraint, because the User object from 
the first test wasn't removed.

Is this a design decision? Or a technical limitation? (I realize that the 
use of a setup classmethod could cause issues with parallelization, but 
those issues could be addressed in the test runner.)

And in the meantime, is there a better solution than to put all the common 
setup code in the setUp method?

Note that I don't want to use fixtures for this. Fixtures are not a good 
way to create test data, as they're very static and need to be updated with 
the code. This also makes them error-prone. Instead I'm using Model Mommy (
https://github.com/vandersonmota/model_mommy), a model factory library for 
Django. Another good alternative would be Factory Boy. These factories are 
also the reasons why I don't want to do manual cleanup in tearDownClass: 
First of all the model factory also creates related models that are needed 
to satisfy some foreign key constraints. I don't have references to all 
those model instances. And the second reason is that I think the database 
cleanup is something that Django should handle, not me.

Danilo

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Anyone have ideas on #16550 - custom SQL before/after syncdb?

[Danilo Bargen]

As a sidenote, there was a discussion about this on this mailing list a few 
months ago:

https://groups.google.com/forum/#!searchin/django-developers/16550/django-developers/xN0aMzrmA1Y/KsrsEf7XOA8J

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: review request for anyone who likes regular expression (increasing URLs accepted by URLValidator)

[Danilo Bargen]

Thanks Tim for bringing that up :)

The previous URLValidator would consider some valid URLs as invalid.
That can cause a lot of problems, most people would expect an URL
Validator to return some false negatives sometimes (invalid URLs
considered valid) but not false positives (valid URLs considered
invalid). Furthermore, there are probably a few other projects that depend
on Django's URLValidator (http://stackoverflow.com/q/7160737/284318).
Therefore I consider the current behavior a bug and would love to see
this in Django 1.8.

The ticket discussion can be found 
here: https://code.djangoproject.com/ticket/20003

Danilo

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/47255f36-00c1-4a09-bdfd-5bf0860322fd%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Merge policy for cleanup commits

[Danilo Bargen]

Hi there

(I tried to find previous threads concerning the same topic, but could not 
find any. If this is already the n-th discussion about the topic, sorry 
about it.)

I don't quite agree with the cleanup commit policy stated in the docs:

Systematically remove all trailing whitespaces from your code as those add 
> unnecessary bytes, add visual clutter to the patches and can also 
> occasionally cause unnecessary merge conflicts. Some IDE’s can be 
> configured to automatically remove them and most VCS tools can be set to 
> highlight them in diff outputs. Note, however, that patches which only 
> remove whitespace (or only make changes for nominal PEP 
> 8conformance) are likely to be 
> rejected, since they only introduce noise 
> rather than code improvement. Tidy up when you’re next changing code in the 
> area.


I did such a PR today (without knowing that policy): 
https://github.com/django/django/pull/1345

While I agree that small PRs which fix issues like whitespace should not 
necessarily clutter up the commit history, I disagree for larger cleanup 
commits. In some places the code has serious formatting issues (e.g. lines 
that are indented 3 instead of 4 characters and that only work thanks to 
the lax Python indentation parsing). Also, I disagree that 1 commit which 
cleans up a file would "clutter" its commit history.

While I support fixing coding standard issues on-the-go, I think that it 
clutters up the commit history in a worse way than a cleanup commit would, 
because the commits are not strictly focused on the feature or bug they 
concern.

In addition to the PEP8 changes there were also a few pyflakes changes that 
go beyond simple whitespace formatting. For example in the core module 
there were a few places where "variable == None" was used, even though 
"variable is none" should be used preferredly. Another issue would be 
"type(c) == Typeclass" instead of "isinstance(c, Typeclass)".

Anyways, if you don't want to accept such commits that's OK, but I think 
adherence to coding standards is pretty bad in many Django modules and it 
should be fixed. And for sure I won't be the last person to send you such a 
pull request.

Danilo

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Merge policy for cleanup commits

[Danilo Bargen]

> I can't speak for other core devs, but I won't merge such PRs for a very
> simple reason: it's more tedious and time-consuming to review them than to
> redo them by myself.

Even though there are many changes, the changes are very obvious and
simple and quick to review.

> If someone took advantage of a huge "style cleanup" diff to slip in a
> security vulnerability — and trust me, it doesn't take much code — I
> wouldn't want to have my name on the commit.

That's right. But if you scroll through the diff on Github it's very
easy to see that there are no "deep" changes... Probably 95% of the
changes are whitespace anyways.

> Like the 1400 or so tickets currently open in Trac :)

A cleanup doesn't mean that other tickets should not be fixed :)

> You aren't the first one either. For some reasons I don't quite
> understand, "hey, your coding standards suck, mine are better" is a common
> first-contact technique :)

Well, it's actually your coding standard:
https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/
And it's not my first contribution to Django either. It's just
something that apparently bugs many people and that would be easy to
fix.

> The most important thing to remember though is that this is code
> which works, which is fundamentally the most important thing.

Yes, practicality beats purity, but at the same time beautiful is
better than ugly, sparse is better than dense and readability counts
:)

Cheers
Danilo

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Merge policy for cleanup commits

[Danilo Bargen]

Hm, I'm not sure I understand your e-mail. The link you just posted points 
to exactly the topic we're discussing right now. Maybe you meant to post 
another URL?

Cheers,
Danilo

On Wednesday, October 9, 2013 9:11:09 PM UTC+2, Tomáš Ehrlich wrote:
>
> Relevant discussion 
>
> https://groups.google.com/forum/#!msg/django-developers/tcNVvbAv4-M/bs0zPNLqv48J
>  
>
>
> Cheers, 
>   Tom
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/d0957cc5-8177-448f-b7c4-110137987304%40googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Requiring GitHub login for actions on Trac

[Danilo Bargen]

I just discovered this change (require Github for login) today and had a 
hard time finding this discussion. Maybe a link in the wiki would help?

While I agree with the decision to move on to Github Login (finally no more 
basic auth!) I'd like to have a way to merge my two accounts. Especially 
the ticket history and the notification settings. Right now there seems to 
be no way to turn off notification for tickets that are being tracked with 
the "old" account. While I use the same username (and AFAIR even e-mail) on 
both accounts, the accounts apparently haven't been merged.

Is there a way to merge accounts? Maybe by writing an e-mail to someone? 
And if yes, could we put that in the wiki?

Cheers
Danilo

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/aac1cac5-3b8f-4589-8c04-0d0acbce30ad%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Proposal new Django Admin css style

[Danilo Bargen]

Just for the record, there's also
https://github.com/pydanny/django-admin2, another project that attempts
to create a more modular and extensible admin.

Danilo

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/1408139451.3608429.153233665.2803E551%40webmail.messagingengine.com.
For more options, visit https://groups.google.com/d/optout.