As noted in the PR there is at least one new setting, CHANNEL_SESSION_ENGINE, which is lacking documentation. 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?
There are tests for the routing and request handling but there are no unittests that I can see for the auth/session handling, Channel/Message/Group/ChannelLayerManager/custom JSON encoder classes and their usage, or the worker/runworker code. The changes to runserver aren't tested. The new test utilities aren't tested either (yes that's a thing). All new code should have tests. Isn't that Django's current standard? 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. 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. -Mark 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 > <javascript:>> 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-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/0b519047-96ee-4ac9-9230-0f487625a7e2%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
