I can (and do) appreciate the effort that's gone into this work while still feeling as though it isn't ready to be merged with Django. To be honest given that this PR contains almost no changes to Django itself and only adds new code, I don't understand why it can't live outside of Django while it continues to mature.
Best, Mark On Wednesday, May 4, 2016 at 3:18:03 PM UTC-4, Andrew Godwin wrote: > > > > On Wed, May 4, 2016 at 11:52 AM, Mark Lavin <markd...@gmail.com > <javascript:>> wrote: > >> >> >> Given that there is no guarantee of message delivery, I don't think this >> is suitable or recommended for using for background tasks. I don't think >> the Django docs should encourage that usage. At least without a strong >> warning that this isn't a good idea. >> >> >>> >>>> There are tests for the routing and request handling but there are no >>>> unittests that I can see for the auth/session handling, >>>> >>> >>> Yes, these are missing. >>> >>> >>>> Channel/Message/Group/ChannelLayerManager/custom JSON encoder classes >>>> and their usage, >>>> >>> >>> The ASGI conformance test suite takes care of this: >>> https://github.com/andrewgodwin/django/blob/channels/tests/channels_tests/test_database_layer.py#L3 >>> >>> (that's also used for the inmemory module and asgi_redis in their test >>> suites) >>> >> >>> >> >> Yes the code is covered by integration/functional tests but again I feel >> like there should be unittests for this as well. >> > > Those are unit tests as far as I'm concerned, and they're quite > comprehensive - there's no smaller units of work to really write tests > _for_ in any of these backends. > > >> >> >>> or the worker/runworker code. >>>> >>> >>> What would you suggest these tests look like? They can't spin up a >>> worker and fire requests at it, and most of the worker code is interactive. >>> >> >> It is certainly possible to fire up a worker in a subprocess and test it >> that way. Or you could create an instance of the Worker object and call its >> methods. >> > > I'll look into this, but firing up subprocesses won't work as it suddenly > introduces Redis as a test dependency (right now all tests use the > in-memory backend). > > >> >> >>> >>> >>>> The changes to runserver aren't tested. >>>> >>> >>> Runserver in Django only has a single test, presumably for the same >>> reason; I don't know how I can write a test for this considering the >>> autoreload and extra process mechanisms we'd want to cover. >>> >> >>> >>>> The new test utilities aren't tested either (yes that's a thing). >>>> >>> >>> I would say that the usage of those utilities in other tests means they >>> are tested sufficiently, given that they're used sufficiently; writing a >>> test for just the channel test case would look very similar to the basic >>> tests elsewhere. >>> >> >> The existing test utilities (test client, asserting the number of >> queries, overriding settings, etc) have tests of their own >> https://github.com/django/django/blob/master/tests/test_utils/tests.py >> as do the extensions added by contrib.staticfiles. I don't see why Channels >> is special and therefore doesn't need them. >> > > I would counter that by saying that the Channels test handler code is only > about 20 additional lines of logic and no new assertions, so the existing > suite is more than enough. > > >> >> >>> >>> >>>> All new code should have tests. Isn't that Django's current standard? >>>> >>> >>> There's no clear standard, the docs just say the tests should "exercise >>> all of the new features". I admit that auth/session needs tests - I'll get >>> those done this week - but I don't see how we can reasonably test the >>> interactive commands in an automated fashion given the current test harness >>> (and lack of tests for things like runserver) in Django. >>> >> >> To me it's a bad code smell when code is hard to test. I can't believe >> I'm having to convince anyone in this community about the value of testing. >> > > Well, it is hard to test, mostly due to the fact that testing websockets > requires the test client to also be asynchronous as well as the server, and > if they're in the same process that leads to trouble. > > I am very much aware of the value of testing, but I'm saying that in this > case it's a large hurdle to climb, I believe the tradeoff is acceptable to > merge it in for the alpha as it is and then get the MOSS program to pay > some people to work on just this part so it gets the right level of > attention. > > >> >> >>> >>> >>>> appreciate >>>> asgiref has one "basic" test around WSGI handling which the usage is >>>> documented in this change. I don't think anyone would dare say that covers >>>> potential edge cases in WSGI/HTTP handling. Not test related, the status >>>> code map is also missing many HTTP status codes. >>>> >>> >>> The WSGI handling part of asgiref is not considered complete, and I will >>> probably remove the section about using it in the main Django docs. Only >>> the inmemory backend should be considered used by Django. >>> >> >>> >> >> This is exactly my point. This project is not complete. That is not at >> all clear from this PR or from looking at the asgiref README which only >> re-enforces my feeling that this dependency should not be a requirement >> when installing Django. >> > > Of course the project isn't complete, if it was I wouldn't be trying to > merge it in for an alpha; would you feel happier if I upped asgiref to > version 1.0 and stuck a comment on the wsgi part saying it's not done? > > Please appreciate that I've spent a hell of a lot of time getting the > project to this point, and anything missing is not because of malice or > incompetence but because I don't believe that 100% test coverage is a > necessary prerequisite for getting something merged in, merely sufficient > test coverage, and I'm trying to strike a balance between getting this > actually done in my spare time and getting it landed in 1.10. > > Sessions and auth were definitely in need of tests (I just hadn't found > time yet), so I've pushed up a session test suite today, and I'll look at > auth later on. I think the rest, however, is sufficiently in line with the > way Django currently is tested that we should merge it in and work on tests > for the new (and old!) runserver during the run-up to the release - we have > the funding to pay people to do this, it's just taken longer to get > everything set up to pay people than I would have liked. > > Andrew > -- 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 https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/5bf5537e-0d6f-4803-b391-7824d61d4ac7%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
