Skip to content

Adding the Django Discovery TestRunner #17365 #319

Closed
wants to merge 23 commits into from

6 participants

@myusuf3
myusuf3 commented Sep 4, 2012

This pull request adds the unittest2 test discovery to Django while specifying location for look up in settings.py

That being said, I have only updated the 1.5.txt release until the code is reviewed. Once that is complete I will update the testing documentation and what other documentation that needs to be updated as per discussion had here.

Also let me know if I can improve the testing. I think I have tested the discovery test runner well but if it can be improved to cover edge cases let me know.

Many thanks to @freakboy3742 for his guidance.

https://code.djangoproject.com/ticket/17365 is the attached ticket

@mjtamlyn
Django member
mjtamlyn commented Sep 4, 2012

I'm guessing there's a ticket this is associated to?

Also is the intention to replace the old test runner with the DiscoverRunner? Not that I want it, I tend to use DiscoverRunner anyway, but some people may want the old behaviour

@jezdez
Django member
jezdez commented Sep 4, 2012

FWIW, given this is a copy of the django-discover-runner code and the feature in unitttest2 is called "discover", I'd prefer to use the name "DiscoverRunner" instead of "DiscoveryRunner".

@jezdez
Django member
jezdez commented Sep 4, 2012

Also, the defaultTestLoader must be imported like in the change jezdez-archive/django-discover-runner@0fdc131 since we wrap an installed unittest2 in the django.utils.unittest module and importing from django.utils.loader is the one included in Django. Not doing so prevents an isinstance check in unittest2 from succeeding, leading to the symptoms as mentioned in jezdez-archive/django-discover-runner#2.

@jezdez
Django member
jezdez commented Sep 4, 2012

As to whether we should support the old test runner, I strongly suggest to deprecate our own test runner, making the DiscoverRunner the default in Django>=1.5 projects, switching to the DiscoverRunner by default in 1.6, and remove the old runner in 1.7.

@myusuf3
myusuf3 commented Sep 4, 2012

@mjtamlyn I have updated the pull request to refer to the ticket.

@jezdez Those changes can be made. That is the planned depreciation schedule in place for this.

@jacobian
Django member
jacobian commented Sep 7, 2012

FTR, I'm +1 on @jezdez's proposal.

@jezdez
Django member
jezdez commented Sep 9, 2012

Woot!

@akumria akumria commented on an outdated diff Sep 11, 2012
django/conf/global_settings.py
@@ -293,7 +293,7 @@
# Maximum size, in bytes, of a request before it will be streamed to the
# file system instead of into memory.
-FILE_UPLOAD_MAX_MEMORY_SIZE = 2621440 # i.e. 2.5 MB
+FILE_UPLOAD_MAX_MEMORY_SIZE = 2621440 # i.e. 2.5 MB
@akumria
akumria added a note Sep 11, 2012

Is it necessary to have the PEP8 fixes in this pull request? I'm not sure what the recommendation is for the Django repository, but in my own repositories I ask for either a PEP8 fix before the code changes, or one just after (to separate out changes which should have no functional impact).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@akumria akumria commented on an outdated diff Sep 11, 2012
django/conf/global_settings.py
@@ -346,11 +346,11 @@
# http://docs.python.org/library/datetime.html#strftime-behavior
# * Note that these format strings are different from the ones to display dates
DATE_INPUT_FORMATS = (
- '%Y-%m-%d', '%m/%d/%Y', '%m/%d/%y', # '2006-10-25', '10/25/2006', '10/25/06'
- '%b %d %Y', '%b %d, %Y', # 'Oct 25 2006', 'Oct 25, 2006'
- '%d %b %Y', '%d %b, %Y', # '25 Oct 2006', '25 Oct, 2006'
- '%B %d %Y', '%B %d, %Y', # 'October 25 2006', 'October 25, 2006'
- '%d %B %Y', '%d %B, %Y', # '25 October 2006', '25 October, 2006'
+ '%Y-%m-%d', '%m/%d/%Y', '%m/%d/%y', # '2006-10-25', '10/25/2006', '10/25/06'
+ '%b %d %Y', '%b %d, %Y', # 'Oct 25 2006', 'Oct 25, 2006'
+ '%d %b %Y', '%d %b, %Y', # '25 Oct 2006', '25 Oct, 2006'
+ '%B %d %Y', '%B %d, %Y', # 'October 25 2006', 'October 25, 2006'
+ '%d %B %Y', '%d %B, %Y', # '25 October 2006', '25 October, 2006'
@akumria
akumria added a note Sep 11, 2012

likewise ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@akumria akumria commented on an outdated diff Sep 11, 2012
django/test/simple.py
@@ -110,7 +110,7 @@ def build_test(label):
try:
if issubclass(TestClass, (unittest.TestCase, real_unittest.TestCase)):
- if len(parts) == 2: # label is app.TestClass
+ if len(parts) == 2: # label is app.TestClass
@akumria
akumria added a note Sep 11, 2012

as before ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@akumria akumria commented on the diff Sep 11, 2012
django/utils/unittest/loader.py
@@ -24,6 +24,7 @@ def _CmpToKey(mycmp):
class K(object):
def __init__(self, obj):
self.obj = obj
+
@akumria
akumria added a note Sep 11, 2012

again with this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myusuf3
myusuf3 commented Sep 30, 2012

so documentation here all that needs to be done? I would like to get this in before the freeze if possible. @jezdez ?

@jezdez
Django member
jezdez commented Oct 4, 2012

Yeah, the code looks good to me, doc updates could easily land after the fact, IMO.

@myusuf3
myusuf3 commented Oct 5, 2012

@jezdez so @carljm said that is probably wont get into 1.5 it needs more review and adds too many setting to the setting.py file.

So I guess that's it for now.

@carljm
Django member
carljm commented Oct 5, 2012

Sorry I haven't done a fuller comment on this sooner. I worked on this some at the DjangoCon sprints and talked it over quite a bit with Russ, Jacob, and others. I have some work locally but haven't pushed it yet since it's not yet in a fully working state. I'll try to clean that work up and get it pushed soon. Specifically the things that IMO are not ready yet with this patch:

  1. @jacobian wanted to avoid adding any new settings with this; things like the test discovery pattern should be set instead as command-line arguments.

  2. If we are deprecating the old runner, it's no good to have the new runner inherit from the deprecated runner; that's just postponing a bunch of work to whoever does the final deprecation removal, deprecation removals should not require big code reorganizations. Instead the new runner needs to be standalone with all required functionality, and the old runner should inherit from the new runner, so when it is removed nothing else is affected.

  3. Docs - I don't think we should commit things without adequate docs and then clean it up later. Writing the docs helps us figure out if we've made mistakes in the API. Docs are a first-class citizen.

  4. The current code does a kind of funny thing where it tries to intelligently decide whether to use discovery or loadTestsFromModule depending what you pass on the command line and what it finds there. I did this because it was functional and what I wanted for an external project, but I'm not sure this approach belongs in Django - this is just going right back down the path of inventing our own unique discovery process. I think we should maybe stick closer to the native behavior of unittest, and discuss with voidspace possible improvements to that behavior. This requires more discussion.

And that's really the key - while I'd like to see this change in Django, it's a significant developer-facing change and I think it's worth taking the time to make sure we do it right. And that means not committing something quick before feature freeze just to get it in, it means plenty of time for consideration and review on django-developers. Unfortunately I didn't have time this summer to drive that, but I'm hoping I (or someone else?) will in the 1.6 cycle.

@carljm
Django member
carljm commented Oct 5, 2012

I meant to also say - many thanks @myusuf3 for getting things rolling with this pull request. I'll leave a note here when I get my WIP pushed, in case you or anyone else want to carry it forward before I get back to it.

@jezdez
Django member
jezdez commented Oct 8, 2012

@carljm Thanks for the summary and update, I agree on all counts and updated the ticket.

@myusuf3
myusuf3 commented Jan 9, 2013

@carljm @jacobian @jezdez so what can we do to get the ball rolling here? I would like to contribute to make sure this gets in;

I am just trying to understand the process here. I found the ticket read the discussion spoke with @freakboy3742 with respect on how to go about solving it and what required tests. I understand having different ideas on how to go about things and having a discussion. I am just trying to understand what the process is.

@carljm
Django member
carljm commented Jan 9, 2013

Hi @myusuf3 - my apologies, I screwed up the process by saying here that I'd clean up and push my work-in-progress, and then never actually doing so. I think I outlined in a comment above what I think needs to be done in order for this to be ready to go in; I still haven't found time to finish or clean up my work-in-progress from the DjangoCon sprints, but I've now pushed it, (as-is and not-yet-working) to https://github.com/carljm/django/tree/ticket-17365

If you'd like to work on this, you can start where I left off if that's helpful. If you have questions about the direction I was heading with that branch, I'm happy to talk it over more here or on IRC. I think there are still some pretty significant design decisions to be made in terms of how the test discovery actually works (item 4 in my comment above; how much, if any, custom extra behavior we add on top of unittest2's default discovery behavior), so I don't think this is just a matter of making some minor code tweaks and getting it merged, it's probably going to merit a proposal and discussion on django-developers to gather feedback on the right behavior.

Again, sorry for throwing a wrench in the works by not following through after my last comments. The process from here on out, as far as I'm concerned, is to address points 1-3 above, make a proposal to django-developers regarding the precise discovery behavior (point 4 above), reach agreement on that and implement it, get review of the patch/pull request (I'm happy to do review at that stage), and then hopefully get it merged!

@myusuf3 myusuf3 closed this May 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.