Re: Design decision for #1625 - Adding traceback to return value from send_robust when error occurs
Awesome, great suggestions both. That's a cleaner API and the implementation itself is even slightly cleaner. I updated the patch and uploaded it to the ticket. If you or anyone else wants to review it and ideally move it forward, that would be excellent. Thanks! -- 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/-/v_5hqdvT3LUJ. 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.
Design decision for #1625 - Adding traceback to return value from send_robust when error occurs
https://code.djangoproject.com/ticket/16245 Here's the quick summary: send_robust() as you know is a special case of send() in the signals framework, which wraps each signal trigger in a try/except block so that an exception in one signal doesn't prevent the rest from firing. This is great, but I've run into a problem when using this in production. The problem is that send_robust() does not capture the traceback at the point of the exception. As a result, though you can see which exception was raised, you can't see the traceback assocaited with that exception. This makes debugging exceptions that occur during send_robust extremely, extremely difficult. I've contributed a patch to resolve this. In order to preserve backwards compatibility, my patch adds a kwarg append_traceback to send_robust() which allows one to optionally ensure the traceback is included. This seemed like the most straightforward solution that steps on the fewest toes, though in my opinion the traceback should be included by default and the old way deprecated. The patch includes full tests and documentation. Right now it's stuck in design decision needed. Hopefully someone wouldn't mind taking a moment to review the ticket and patch and move it to ready for checkin. If there are concerns or questions or the patch need work I'd be happy to do what is required. Many thanks... -- 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/-/HdK-7gZdmSsJ. 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.
New feature: Customizable get_queryset() for django.contrib.comments comment_list template tags
I understand that we're supposed to suggest new features here on django-dev
first instead of opening a ticket, so here goes:
Presently in django.contrib.comments, there are two template tags that
output a list of comments:
{% render_comment_list for [object] %}
{% get_comment_list for [object] as [varname] %}
There is a method on BaseCommentNode called get_query_set() that composes
the queryset for these tags and returns it.
At present, there is no way to customize the queryset. I propose that we
provided a mechanism allowing a custom comment app to define its own
queryset for the list of comments that can be retrieved.
What's wrong with the present way?
* You can't use a custom manager. It assumes you want to use the manager
defined on the objects attribute.
* It assumes you want at all times to apply the moderation fields if they
exist.
* It doesn't allow you to define custom moderation filters, e.g. if you had
a custom field with a score on your comment model and you didn't want to
show comments below a threshold, or if you wanted to exclude comments that
had been flagged as spam.
Is there a workaround?
You can subclass BaseCommentNode and override its get_query_set() method,
and then implement your own template tags. Basically your just having to do
a lot of copying and pasting to get what you need. If there is a more
obvious or easy way of defining this that I have overlooked, do let me know.
Proposed feature
Two possibilities:
1. Define a new function as part of the present Custom comment app API. Call
it something like get_comment_list_query_set(). Implement the present logic
from the get_query_set() method there, and allow it to be overridden in the
__init__.py file of the custom comment app.
2. Extend the template tag to allow a custom queryset to be passed to it.
Something along the lines of:
{% render_comment_list for [object] with [custom_comment_list] %}
{% get_comment_list for [object] as [varname] with [custom_comment_list] %}
Neither of these two are mutually exclusive (i.e. they could both be
implemented). I would think the second provides the most flexibility, but at
the same time I think the first is in strong alignment behind the present
custom comment app API philosophy.
Please let me know your thoughts/concerns/criticisms/etc. I'd be happy to
work on a patch if there is consensus that it's a worthwhile idea. Thanks.
--
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/-/9-6LEMDkYqoJ.
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.
Deferred constraint checking causing problems during test runs
Howdy, I'd like to share some findings I made recently with regards to deferred constraint checks during test runs. There's a bit of background here (https://code.djangoproject.com/ticket/3615) and here(https://code.djangoproject.com/ticket/11665), but this is the gist of what's going on now: As you all know, when the Django test suite runs it executes each test within a DB transaction, which is opened during setUp and rolled back during tearDown. This works great for testing, but there is a problem, which Ramiro brought up the other day on IRC and I observed as well. The problem is that by default in most of our backends, a transaction runs with constraint checks deferred. This means that foreign keys are *not* checked for integrity until a transaction is committed. But here's the rub: we never commit a transaction during tests, we roll it back instead. Therefore, none of the queries against the DB are being checked for integrity. (The exception is MySQL InnoDB, which I'll get to in a sec.) As part of the work I was doing recently to disable and re-enable constraint checks during fixture loading, I realized that we were handling this sort of incorrectly across our backends. Postgresql in fact does provide a facility for enabling constraint checks: SET CONSTRAINT CHECKS ALL IMMEDIATE. This statement causes Postgresql to behave as it normally would, when it was executing queries outside of a transaction. So I tried this out on Postgresql, and what I got when I executed the test suite was: Ran 3947 tests in 570.399s FAILED (failures=53, errors=233, skipped=16, expected failures=2) Interestingly, this looks familiar. Here's the result of the test suite on MySQL with Innodb: Ran 3948 tests in 5171.412s FAILED (failures=67, errors=213, skipped=21, expected failures=2) Pretty darn close. Anyhow, as I started to dig a bit deeper I realized something I considered to be quite important: MySQL, which has sort of been dismissed as being unable to handle constraint checks like a "real" database has in fact been running like a real database all along. The bugs that have accumulated during MySQL tests are in fact real bugs. When Postgresql is configured with constraint checks set to immediate -- and as a result behaves as it would in production -- these very same bugs are revealed. The good news is there are really just a few minor problems triggering all these errors, and most of them are bugs in the test suite rather than production code. As a matter of fact, with a bit of hacking yesterday and today, I managed to get most of this under control. As of right now, when running the test suite in a fixed branch I have, I get: Postgresql: Ran 3993 tests in 946.555s FAILED (failures=3, errors=6, skipped=17, expected failures=3) MySQL: Ran 3994 tests in 4857.777s FAILED (failures=8, errors=7, skipped=25, expected failures=3) In other words, we're in striking distance from getting all tests to pass on all backends, and I uncovered a handful of issues (again, mostly bad test code) which were hidden from sight because constraint checks weren't being run. Right now I've got everything on an experimental git branch, which is still somewhat a work in progress: https://github.com/jsdalton/django/tree/experimental_check_constraints_fixed . As I said, I was mostly hacking away to kill the bugs and cut through the fog, so at this point I don't think what i have is ready to be proposed as a patch. Note that this is a branch off of the work I did on #3615 to fix fixture loading, which is sort of an important underlying piece that had to come together to be able to flush everything out. In brief, here are the issues I uncovered, running with Postgresql. * Content types get flushed from the DB after each test. However, they remain cached in the manager. When constraint checks are working, certain tests were failing because the a Content type wasn't being created in the DB in get_for_model(). Fix was to clear the contenttype cache at the start of every test run. * Some deletion tests were erroring because constraint checks needed to be temporarily disabled during batch deletes. * Big sneaky one that I pulled most of my hair out on until I finally found it...this bad boy: https://github.com/jsdalton/django/commit/b452248a0d9a025c7aa387091fa6d6b1f51113df . This is the reason why there are hundreds of errors on MySQL when the test suite is run on master right now. Basically the monkey patched routers were not being restored properly. As a result, the 'other' database was no longer being used. Tests were failing all over the place for every test that ran after this one. Fixing this single type caused about 200 tests to start passing again in MySQL, and Postgresql as well with constraint checks immediate. The above are the issues I was able to patch up. Other fixes might be desired on the above, but again my goal was just to get tests passing again. There were some
Re: Thoughts on solution to forward references in MySQL (#3615)
Are any core devs interested in taking a closer look at the current patch on this ticket (https://code.djangoproject.com/ticket/3615)? It's been through several rounds of revision after discussion here and on the ticket comment thread. As of right now the patch should apply cleanly and pass tests in fixtures_regress. It solves the problem of IntegrityError exceptions when loading a fixture in MySQL, and it also applies foreign key constraint checks for both MySQL and SQLite to ensure valid data is being loaded. Last, it offers two hooks for other backends to implement constraint checks after fixtures are loaded. The issues I still had questions on: * There is a test error in fixtures_regress.TestFixtures.test_loaddata_raises_error_when_fixture_has_invalid_foreign_key() if a backend does not implement a solution to check for foreign keys. Basically I have MySQL and SQlite covered, but not postgresql or Oracle. As I mention in the ticket, the work done here https://code.djangoproject.com/ticket/11665 covers Postgresql, so all it needs is an implementation from an Oracle user. * There's a DB feature can_defer_constraint_checks . I couldn't find much by way of documentation or or usage of this feature. But I was trying to figure out if the work we are doing here is what this feature refers to, and if so, if we should be marking this as True for MySQL and SQLite with this implementation. I'm not sure there is other behavior that is required or expected in that attribute. Anyhow, that might also be a path forward for the issue I raise above (ie. we could skip the test if can_defer_constraint_checks is not True). I know there is other very similar work being coordinated elsewhere, so hopefully someone with an an eye on the big picture can help me see this through the last mile. Aside from the minor details above, this should be ready for checkin. Thanks! -- 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/-/q_RzrPrYHboJ. 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.
Resetting cache gracefully at the end of every test?
This issue came up again for me recently: Because the cache is not reset or flushed after every test, cache values linger there. This can create headaches when writing tests. I would like to propose that we treat the cache backend(s) like database backends during the test run. Specifically, I propose that we: * Prefix "test_" before the KEY_PREFIX in each backend during global setup. This is equivalent to adding the test_ prefix to DB names. In doing this we prevent cache calls during test form overwriting possible legitimate values. * Monkey patch cache.set() (and cache.set_many()) such that we record all keys used to set values during a test. * Use cache.delete_many() on the list of keys we record being set during test teardown. This will restore the cache to its previous state after each test run. It's better than flush() which clears the whole cache and is too destructive to be run during testing. I don't think it needs to be more complicated than this. I checked around to see if anyone had opened a ticket related to this or had previous discussions on the subject but couldn't find anything. I did briefly touch upon this with Russ in a comment thread on https://code.djangoproject.com/ticket/15026 and I noticed he made a comment in this vein on Django Users: https://groups.google.com/forum/#!topic/django-users/OOGO-3MIO_c . If there is something already open on this or a decision has already been made, please do feel free to point me in the right direction. Otherwise, if there is interest I'd be happy to open a ticket and get a patch started. Thanks Jim -- 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/-/dyMCEKilRuMJ. 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.
Re: Thoughts on solution to forward references in MySQL (#3615)
On Monday, June 27, 2011 7:04:09 PM UTC-7, Jim D. wrote:
So that caveat aside, I will try later to find a large data set I can run
> the query on to get an idea of what kind of performance hit the check
> entails. If anyone else has a large data set with two related tables, you
> can try it out as well and report your results.
Okay, I just ran this on an InnoDB with 228k rows that's about 8.5MB in size
("REFERRING"), with a related table of 248 rows to join on ("REFERRED"),
with no invalid records:
SELECT REFERRING.`id`, REFERRING.`id_pointing_to_some_related_table ` FROM
`some_table` as REFERRING
LEFT JOIN `some_related_table` as REFERRED
ON (REFERRING.`id_pointing_to_some_related_table` = REFERRED.`id`)
WHERE REFERRING.`id_pointing_to_some_related_table ` IS NOT NULL
AND REFERRED.`id` IS NULL
I'm getting this consistently under 1 second, around 700 or 800 ms
generally. Not great but not horrible. I didn't do any sophisticated
profiling or setup for this or anything.
I'm not sure what kind of performance we might think is acceptable, but for
the dominant use cases of load data I think it's fine.
My thinking at this point would be the performance is "good enough" for the
scope of the current ticket, and that if better performance were required or
desired, that could be facilitated under a separate ticket, assuming there
was community demand for it.
--
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/-/hynqngB0OHMJ.
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.
Re: Thoughts on solution to forward references in MySQL (#3615)
On Monday, June 27, 2011 3:08:03 PM UTC-7, Jacob Kaplan-Moss wrote: > > I left some comments on the patch on the ticket. > Excellent just saw those. I'll take a look. > > I'm using django-threadedcomments on a project, which has forward > > references in one of its test fixtures, so I'm reminded of this issue > > every time I run my project tests. I hate test errors! I'm hopeful the > > latest patch is sufficient to get this issue resolved. > > Does this patch work for you (I'm assuming so)? A "this works for me > in production" goes a huge way towards me feeling comfortable checking > things in. > Does it work in production is a hard question for me to answer, if I understood your question properly. In my projects, I really only touch the loaddata command when I'm running tests (I guess loading data via initial_data.json into a production DB sort of counts as running in production). I don't think anything in my proposed changes touches production code with possibly a few minor exceptions. I know that I can run the full Django test suite with MySQL in innodb and I do not get any fixture loading issues related to forward references. It'd be great if some others could test this patch out as well. In particular, I can't be sure how well ti works on older versions of MySQL. There's no crazy magic at work here but, well, you never know. If it works for others and we're all in agreement with the philosophical approach of the solution, I think it should be a fairly uncontroversial commit, since it's really primarily a fixture loading issue at its core. -- 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/-/NapNIk_SmOcJ. 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.
Thoughts on solution to forward references in MySQL (#3615)
Hi all, I spent some time last week and over the weekend nailing down a solution for https://code.djangoproject.com/ticket/3615 . This is the ticket about allowing forward references when loading data on the MySQL InnoDB backend. My patch implements the proposed change (disabling foreign key checks when the data is loaded) as well as a straightforward SQL SELECT check for integrity after the data is loaded, which if I understand it is the missing piece that has prevented this ticket from moving forward for the last 4 years... Anyhow, the patch should be 100% there so I'd love if someone could check it out and either push it along or let me know if any changes are required. It should be easy enough for me to address any issues while the whole problem is in my head. I'm using django-threadedcomments on a project, which has forward references in one of its test fixtures, so I'm reminded of this issue every time I run my project tests. I hate test errors! I'm hopeful the latest patch is sufficient to get this issue resolved. Thanks Jim -- You received this message because you are subscribed to the Google Groups "Django developers" group. 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.
Re: Re : Re: #1342 Allow customization of MAX_SHOW_ALL_ALLOWED. Reopen or new ticket?
On May 11, 9:47 pm, Mathieu AGOPIAN wrote: > Just to make sure i've understood the topic here: you need to change > MAX_SHOW_ALL_ALLOWED, but only for a specific model? > > Otherwise you could just add something like that to, say, the root urlconf: > > from django.contrib.admin.views import main as admin_views_main > > admin_views_main.MAX_SHOW_ALL_ALLOWED = 1000 Thanks Mathieu. That's actually a fine little hack for overriding the default site-wide. If I'd thought of that in the first place I probably wouldn't have gone through the trouble of bringing up this issue. :) As it were, I ended up opening a ticket submitting a patch that converts it into an configurable attribute on ModelAdmin: http://code.djangoproject.com/ticket/15997 The patch I attached there also adds some documentation and test coverage for the show all feature, which is lacking at the moment, so hopefully it represents an improvement on what we have now. -- You received this message because you are subscribed to the Google Groups "Django developers" group. 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.
#1342 Allow customization of MAX_SHOW_ALL_ALLOWED. Reopen or new ticket?
I'm looking at a five-year-old ticket here (http:// code.djangoproject.com/ticket/1342) that suggests MAX_SHOW_ALL_ALLOWED in the admin be configurable. As of now it is hard coded at 200 in contrib/admin/views/main.py . The ticket is, in my opinion, somewhat erroneously marked as "fixed", as someone pointed out 14 months ago. The patch that supposedly fixed it is the one that added the list_per_page attribute; however, that patch only addressed half of the issue (ability to configure results per page) and not the other half related to the show all. Clearly this is not an issue of overwhelming importance or I'd imagine it would have already come up in the last five years. Be that as it may, I have an important use case where I'd like to preserve pagination but allow for a show_all link to appear, which can only be accomplished gracefully by changing the MAX_SHOW_ALL_ALLOWED. It also strikes me as bad form to hard code an arbitrary value like that without providing any recourse to amend it. First question is administrative -- should I reopen this old ticket or start a new ticket? Second question is how to approach the patch. Is it acceptable to move this to global_settings under an "Admin" section at the bottom and leave it undocumented? Given that it's already undocumented and there are other settings in global_settings that are also undocumented, this would seem to be the quickest approach that would hopefully ruffle the fewest feathers. If that's not kosher, I'd be happy to contribute a patch with full documentation if need be, or to take a different approach, e.g. make this configurable at the site or model admin level. Third question, is there some reason this issue has not been fixed before or should not be fixed? Mostly I just want to make sure this issue has not been discussed before and some conclusion reached that I'm not aware of (searched, but couldn't find anything). If anyone has any thoughts regarding any of these questions I'd appreciate hearing them. My main objective is to find the most direct path to something that you guys will find acceptable. Thanks. -- You received this message because you are subscribed to the Google Groups "Django developers" group. 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.
Re: Questions about possible regression in recent changeset addressing cache
Thanks for the detailed explanation Russ. I was mainly concerned about jumping into to attempt to rework a few things without a better understanding of what the code is supposed to be doing. If I find a few minutes today, I was thinking about writing a few tests specifically against the __init__ method of CacheMiddleware to ensure the right attributes are being set in the right way going forward, and then perhaps proposing a few minor tweaks to to the code to straighten things out. I'll put up a patch if I manage to get anywhere. Jim On Jan 23, 3:33 am, Russell Keith-Magee wrote: > On Sat, Jan 22, 2011 at 2:27 PM, Jim D. wrote: > > Howdy, > > > I reported a bug earlier today (http://code.djangoproject.com/ticket/ > > 15144) related to a possible regression in changeset 15021 (http:// > > code.djangoproject.com/changeset/15021). The apparent regression is > > that as of this changeset, the cache middleware no longer respects the > > timeout value passed to the @cache_page decorator. > > > I found some time this evening to further research this. I was able to > > create a regression test that demonstrated the bug. However, in trying > > to create a patch which addresses the bug I'm having a hard time > > making sense of what changeset 15021 set out to accomplish. > > > For whatever reason, I am unable to find any discussion related to it > > on this discussion list or in trac. If someone would be kind enough to > > direct me to discussion surrounding this changeset or the ticket(s) > > associated with it, I would be most appreciative. > > To fill in some of the blanks here; r15021 was an attempt to improve > the testing of some of the features introduced in r15005, when > multiple cache backends were introduced. > > In particular, I was concerned about the argument handling in the > cache_page decorator. The cache_page decorator is just a wrapper > around the combined cache middleware; my concern fell into two areas: > > * options passed to the decorator wouldn't be used correctly by the > underlying implementation > * Unspecified options on the decorator would fall back to the wrong > defaults (middleware-specific defaults, rather than decorator > defaults) > > The test cases that are contained in r15021 cover the areas that I was > concerned about. > > > Anyhow, that said, it appears to me there are some minor issues with > > the code in the __init__ method of CacheMiddleware and there does not > > appear to be adequate test coverage around some of the code. > > Entirely possible. Like I said, I added a bunch of tests that covered > cases that I could see would be a problem (or were problems, and were > fixed by the rest of the patch). I make no claim that the rest of the > test suite is adequate. > > > * The cache_timeout value is being passed along to the cache, but > > self.cache_timeout is not being set. This is the problem I identified > > in the ticket I filed. The value was previously being set prior to > > changeset 15021. Why was this removed and why is it now being set to > > the default_timeout of the cache? > > As I noted in the ticket, it wasn't removed; the logic just gets > invoked in a different way. > > It appears that you've now provided a test case, and Jag has > volunteered a patch; I'll be sure to take a look at both in the near > future. > > > * There appears to be an attempt to determine which alias and prefix > > to use in the try / except blocks. The problem is that the key is > > being accessed via a dict get() method, which will never raise a > > KeyError (http://docs.python.org/library/stdtypes.html#dict.get), it > > will only return None. So basically the except blocks are never going > > to be raised. This could probably be resolved by passing a default > > value of e.g. False as the second arg to get() in each instance, and > > then changing the try/except block to a condition. It also does seem > > like some tests are needed here to properly run these various code > > paths and make sure they are being set correctly. > > This is probably a case of code evolving against a deadline. At the > time r15021 was committed, I was working against the alpha deadline, > and I wanted to make sure that the feature added a couple of days > earlier didn't have any major bugs. As a result, the implementation > evolved quickly, and it's entirely possible a couple of facepalm > errors slipped in. > > > * It seems kind of weird to me that if a 'cache_alias' is set > > explicitly, then we don't fall back to CACHE_MIDDLEWARE_SECONDS if a > > cache_timeout is not found, but if it's
Questions about possible regression in recent changeset addressing cache
Howdy, I reported a bug earlier today (http://code.djangoproject.com/ticket/ 15144) related to a possible regression in changeset 15021 (http:// code.djangoproject.com/changeset/15021). The apparent regression is that as of this changeset, the cache middleware no longer respects the timeout value passed to the @cache_page decorator. I found some time this evening to further research this. I was able to create a regression test that demonstrated the bug. However, in trying to create a patch which addresses the bug I'm having a hard time making sense of what changeset 15021 set out to accomplish. For whatever reason, I am unable to find any discussion related to it on this discussion list or in trac. If someone would be kind enough to direct me to discussion surrounding this changeset or the ticket(s) associated with it, I would be most appreciative. Anyhow, that said, it appears to me there are some minor issues with the code in the __init__ method of CacheMiddleware and there does not appear to be adequate test coverage around some of the code. Namely: * The cache_timeout value is being passed along to the cache, but self.cache_timeout is not being set. This is the problem I identified in the ticket I filed. The value was previously being set prior to changeset 15021. Why was this removed and why is it now being set to the default_timeout of the cache? * There appears to be an attempt to determine which alias and prefix to use in the try / except blocks. The problem is that the key is being accessed via a dict get() method, which will never raise a KeyError (http://docs.python.org/library/stdtypes.html#dict.get), it will only return None. So basically the except blocks are never going to be raised. This could probably be resolved by passing a default value of e.g. False as the second arg to get() in each instance, and then changing the try/except block to a condition. It also does seem like some tests are needed here to properly run these various code paths and make sure they are being set correctly. * It seems kind of weird to me that if a 'cache_alias' is set explicitly, then we don't fall back to CACHE_MIDDLEWARE_SECONDS if a cache_timeout is not found, but if it's not set then we do fall back to that setting? If someone can shed some light on these questions, I can hopefully set aside some time to write a few tests and rework the code a bit if it does turn out that is needed. Many thanks... -- You received this message because you are subscribed to the Google Groups "Django developers" group. 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.
Quick review of #15026
Howdy, Just wanted to see if anyone wanted to do a quick review of the patch I submitted for #15026: http://code.djangoproject.com/ticket/15026 The ticket in question refers to a failure in contrib.session tests when memcached is specified as the cache backend. i had suggested clearing the cache as a solution, and Russell had moved the ticket to "Ready for checkin" in response. However, upon further reflection I realized that clearing the cache clears the entire cache and this probably wasn't desirable behavior for the test suite, so I proposed an alternative. This is not a super critical bug, but I just wanted to make sure it gets reviewed before the 1.3 deadline and hopefully queued for checkin. -- You received this message because you are subscribed to the Google Groups "Django developers" group. 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.
Re: Proposal: Add signals test_setup and test_teardown to Django test suite runner
Cool, thanks On Sep 20, 3:30 pm, Carl Meyer wrote: > Thanks for your work on this! The usual Django workflow doesn't > include patches to the mailing list: rather you can go ahead and open > a Trac ticket and attach the patch there (even if you aren't sure of > the approach yet), and reference the ticket number here. Cool, thank you Carl. I'll go ahead and submit it as a ticket and let it take its natural course from there. > So where this would break is if someone is doing a bit too much at > import time of their tests module (like in the class body of a > TestCase subclass): for instance, saving something to the database. I > have to admit (gulp) that I have actually done this before and it > currently works fine, but it's not really good practice and it's > certainly not documented anywhere that you can do that, so personally > I'm not sure it would be a problem to break it. Yeah, that's what I was thinking too. Maybe other people will have examples where it might cause some critical issues, I guess we'll see. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@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.
Re: Proposal: Add signals test_setup and test_teardown to Django test suite runner
I found some time this evening to work this out, and have included a revised patch for this proposal at the end of this message. I tried to keep changes to an absolute minimum, but here are a few notes about the changes and decisions I did make: * The addition of this feature required what I imagine to be a significant change to test suite runner. Namely, I switched the order of the build_suite() and setup_test_environment() methods in run_tests(). The reason for this has to do with what i suggested in my earlier reply in this thread, namely that there isn't a feasible way to connect to the signals from application code until after applications have been imported. I also wanted to attempt to implement Russ's suggestion of having Django itself use the signals for setup and teardown. The most logical and clean way to do this (and the only way to organize things such that Django could use the code) was to construct the run_test() sequence such that test suites are first built, then setup, then run tests, then teardown. The Django tests suite passed when the order was inverted, but I am quite certain it is a huge assumption on my part that the order can be switched here. Does anybody have any opinions on this, or reasonings as to why the setup_test_environment() stuff would need to be called before build_suite()? I imagine this would be the greatest concern with my proposed patch. * The test utility methods setup_test_environment() and teardown_test_environment() are now connected to the respective signals and are executed when those emit, rather than being called directly from the test suite runner. The **kwargs was added to their signature so that the they could be executed via the signal. * Aside from adding the code to create the signals and to emit them in the test runner, that's all the changes that were necessary to implement this feature. * I have included documentation changes in the patch in what is hopefully a tolerable first stab. I welcome any additional feedback or suggestions. Provided there are no serious objections or deal breakers pointed out in this discussion, I'll submit a ticket for this, which I assume is the best way to move it forward in the official discussion and review process. Here's my revised proposed patch: Index: django/test/simple.py === --- django/test/simple.py (revision 13861) +++ django/test/simple.py (working copy) @@ -7,6 +7,7 @@ from django.test import _doctest as doctest from django.test.utils import setup_test_environment, teardown_test_environment from django.test.testcases import OutputChecker, DocTestRunner, TestCase +from django.test.signals import test_setup, test_teardown # The module name for tests outside models.py TEST_MODULE = 'tests' @@ -230,7 +231,7 @@ self.failfast = failfast def setup_test_environment(self, **kwargs): -setup_test_environment() +test_setup.send(sender=self) settings.DEBUG = False def build_suite(self, test_labels, extra_tests=None, **kwargs): @@ -284,7 +285,7 @@ connection.creation.destroy_test_db(old_name, self.verbosity) def teardown_test_environment(self, **kwargs): -teardown_test_environment() +test_teardown.send(sender=self) def suite_result(self, suite, result, **kwargs): return len(result.failures) + len(result.errors) @@ -308,8 +309,8 @@ Returns the number of tests that failed. """ +suite = self.build_suite(test_labels, extra_tests) self.setup_test_environment() -suite = self.build_suite(test_labels, extra_tests) old_config = self.setup_databases() result = self.run_suite(suite) self.teardown_databases(old_config) Index: django/test/signals.py === --- django/test/signals.py (revision 13861) +++ django/test/signals.py (working copy) @@ -1,3 +1,5 @@ from django.dispatch import Signal template_rendered = Signal(providing_args=["template", "context"]) +test_setup = Signal() +test_teardown = Signal() \ No newline at end of file Index: django/test/utils.py === --- django/test/utils.py(revision 13861) +++ django/test/utils.py(working copy) @@ -52,7 +52,7 @@ return self.nodelist.render(context) -def setup_test_environment(): +def setup_test_environment(**kwargs): """Perform any global pre-test setup. This involves: - Installing the instrumented test renderer @@ -71,8 +71,9 @@ mail.outbox = [] deactivate() +signals.test_setup.connect(setup_test_environment) -def teardown_test_environment(): +def teardown_test_environment(**kwargs): """Perform any global post-test teardown. This involves: - Restoring the original test renderer @@ -89,6 +90,7 @@ del mail.original_email_backend
Re: Proposal: Add signals test_setup and test_teardown to Django test suite runner
Thanks for the suggestions and guidance Russ. I'll work with it a bit more and see if it pans out. My main concern with this working smoothly in the core has to do with when and how application code is loaded in the process of setting up tests. It turns out the earliest we can realistically send the test_setup signal is in the build_suite() method, right after the get_app() / get_apps() functions have executed, i.e. the applications themselves have been loaded. Prior to this any attempts to connect() to the signal won't get picked up, because no applications have been loaded. There's also a related issue, which is that if you run individual tests or sets of tests for an application, it doesn't appear that other applications get loaded. This means that, in a given application, I might add a hook and expect it to run whenever tests for the project are run, but it wouldn't in fact run unless that application was included in the test. So e.g. if I wanted to disable certain behavior whenever tests were run, that might not in fact happen. That said, I guess if another application includes the application where the hooks live, those connect() calls would get set up at the same time. Not sure if what I'm saying is totally clear, but I guess the point is there are a few more complexities here in the setup process than might first meet the eye. Also, given the above, it might not be realistic to configure the existing setup code to run via this signal, since the signal happens so much later in the process. The solution is probably for me to think this through a bit more and see if there isn't a way to send the test_setup signal where one would expect it should be sent (i.e. at the end of the setup_test_environment() method in the suite runner) and at the same time ensure that applications have a way to connect to the signal. I'll give it some thought. Thank you again for the support and suggestions. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@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.
Proposal: Add signals test_setup and test_teardown to Django test suite runner
I recently asked a question on Django Users related to executing certain code during the global setup and teardown routines that run in Django's test runner. In my particular use case, I was looking for a hook where I could disable some third party API code when tests execute, in much the same way that Django itself swaps out the email backend during testing to ensure no emails are actually sent. Anyhow, it seems the only solution at the time being is set up a custom test runner, which is what I ended up doing. However, it occurred to me that a more elegant approach would be to use signals that run during setup and teardown, which applications could hook into if they needed to do any global setup or teardown actions. To me, this seems like an excellent solution to the problem (it's actually what I implemented in my custom test runner I ended up using). I wonder if this is something that would be considered for addition to the core? I at least wanted to throw it out there for discussions. I've included a patch I wrote to implement this in the core test module, at the bottom of this message. It should be mostly self explanatory. Note that the call to send the test_setup figure has to occur after get_apps() has executed in build_suite(), since applications aren't loaded until then and any signal connections would not yet have had a chance to be set up. Just a few arguments to throw out in favor of this idea: * Requiring a custom test runner to implement this behavior makes it (nearly) impossible for reusable applications to modify global setup and teardown behavior, since it would become the responsibility of the project itself to specify the custom test runner. * The current setup gives the Django core "privileged" access to disable certain features during testing, it would seem that application should be given the capability as well. * Signals are non obtrusive...if they are not used they don't really harm anything. * None of the proposed changes would impact production code, since they are all restricted to the test suite. In fact the patch is really only about 5 lines of additional code. Anyhow, hopefully this is something you guys would be interested in considering. Apologies if this topic has been discussed or proposed before, but in searching I could not find anything related to it. Proposed patch: Index: test/simple.py === --- test/simple.py (revision 13861) +++ test/simple.py (working copy) @@ -7,6 +7,7 @@ from django.test import _doctest as doctest from django.test.utils import setup_test_environment, teardown_test_environment from django.test.testcases import OutputChecker, DocTestRunner, TestCase +from django.test.signals import test_setup, test_teardown # The module name for tests outside models.py TEST_MODULE = 'tests' @@ -246,7 +247,11 @@ else: for app in get_apps(): suite.addTest(build_suite(app)) - + +# This signal can't come any earlier, because applications are actually loaded +# in get_apps() +test_setup.send(sender=self) + if extra_tests: for test in extra_tests: suite.addTest(test) @@ -284,6 +289,7 @@ connection.creation.destroy_test_db(old_name, self.verbosity) def teardown_test_environment(self, **kwargs): +test_teardown.send(sender=self) teardown_test_environment() def suite_result(self, suite, result, **kwargs): Index: test/signals.py === --- test/signals.py (revision 13861) +++ test/signals.py (working copy) @@ -1,3 +1,6 @@ from django.dispatch import Signal template_rendered = Signal(providing_args=["template", "context"]) +test_setup = Signal() +test_teardown = Signal() + -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@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.
