On Wednesday, May 4, 2016 at 12:39:02 PM UTC-4, Andrew Godwin wrote: > > > > On Wed, May 4, 2016 at 8:26 AM, Mark Lavin <markd...@gmail.com > <javascript:>> wrote: > >> As noted in the PR there is at least one new setting, >> CHANNEL_SESSION_ENGINE, which is lacking documentation. >> > > That's been added now. > > >> The notes on deployment for "Running ASGI under WSGI" don't give any >> motivation why you would want to do this given that it doesn't support >> websockets in this case. Is there any advantages? Disadvantages? >> > > It lets you still have background tasks and non-WebSocket interface > servers (or run websocket interfaces separately). I've expanded the docs to > say this. > >
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. > 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. > > >> 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. > > >> 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. > > >> >> 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. > >> I'm not in favor of on leeway for external dependencies for the reason >> you note: they are in the install_requires. Django has avoided having >> required dependencies and I don't think it sets a good precedent to have >> the first set be those which don't meet the quality standards expected by >> the community. That is they don't have sufficient docs/tests to be include >> in Django itself. It isn't clear why these are required if you are claiming >> that they aren't required to run. If these we're in the install_requires >> then I would withdraw the objections about them with regard to this change >> in Django. >> > > With a change I'm going to make, Django will depend on these in > install_requires but not actually use them until you try and use a channels > feature; they're in install_requires out of user convenience (having a > traceback the first time you use the new built-in Django feature seems > bad). Considering we're launching Channels in 1.10 as a "provisional" > feature, my opinion would be that we can go slightly easier on these > dependencies (but of course I'm biased) > > Andrew > > >> >> On Wednesday, May 4, 2016 at 10:26:22 AM UTC-4, Andrew Godwin wrote: >> >>> >>> On Wed, May 4, 2016 at 6:15 AM, Mark Lavin <markd...@gmail.com> wrote: >>> >>>> I had assumed this was still a work in progress because there are >>>> missing tests and some documentation. The build is passing but the >>>> unittest >>>> coverage for the new modules seems low or at least not up to the standards >>>> I expect for Django contributions. The same for the daphne and asgiref >>>> packages which are new requirements. In previous discussions there were >>>> talks about performance benchmarks to track potential regressions before >>>> this would be accepted similar to the template-based widget rendering >>>> change. I don't see any references here or in this work. What is the >>>> status >>>> of those? >>>> >>>> >>> - The documentation on Channels is considerably more than I would have >>> accepted for a patch. What do you think is missing? >>> >>> - The channels branch is likely missing some tests around auth and >>> session in particular, but a lot of the "main" stuff has tests; what would >>> you need before accepting it? >>> >>> - asgiref quite literally has more tests than actual code we're using, >>> so I don't see a problem there >>> >>> - Daphne is severely lacking in tests for WebSocket encoding/decoding >>> support as writing a test harness for it is ridiculously hard and I was >>> hoping to get some help on that now we finally have the MOSS money around. >>> >>> - There are no performance regression tests as by default the codepath >>> in Django is the same as before (apart from runserver, which in my basic >>> tests actually sped up); as long as you deploy using WSGI, you never even >>> touch Channels code (which wasn't the original plan, but now is for sanity >>> reasons). >>> >>> I would like some leeway on the external dependencies being fully tested >>> considering that they are not beholden to Django's release cycle, but maybe >>> because we're declaring them as core dependencies (I patched in >>> install_requires into setup.py for them) we should be very strict? If this >>> patch is going to be denied based on Daphne not having sufficient test >>> coverage then I suspect we'll miss the deadline and have to foist this on >>> the LTS instead, which I really don't want. >>> >>> 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-develop...@googlegroups.com <javascript:>. >> To post to this group, send email to django-d...@googlegroups.com >> <javascript:>. >> 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/0b519047-96ee-4ac9-9230-0f487625a7e2%40googlegroups.com >> >> <https://groups.google.com/d/msgid/django-developers/0b519047-96ee-4ac9-9230-0f487625a7e2%40googlegroups.com?utm_medium=email&utm_source=footer> >> . >> >> For more options, visit https://groups.google.com/d/optout. >> > > -- 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/910b2c17-24fb-450b-acf0-cb027b2aed07%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
