Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ChannelLiveServerTestCase #497

Merged
merged 62 commits into from
Apr 11, 2017
Merged

Add ChannelLiveServerTestCase #497

merged 62 commits into from
Apr 11, 2017

Conversation

proofit404
Copy link
Member

@proofit404 proofit404 commented Jan 23, 2017

As we discussed earlier in #494 we need live server test case for example to run selenium against it. This is work in progress PR, but I want some feedback already.

Few questions we need to answer before merging it.

  1. Channels documentation mention test_channel_aliases as ability to test complex layers setup. I don't know what to do in this case for live server test. It is possible to spawn multiple live server instances. But its not clear to me what user API we should provide.

  2. Regular live server thread contains connection override logic. I don't understand right now what problem does it solve. It will be awesome if any one can describe the reason it present there or provide test case for it.

  3. How we can validate that twisted bind port successfully? For now I select first port from the possible range without any validation.

  4. apply_routes setup test instance with overriding setUp and tearDown methods. Live server spawns its threads in the test class setup (as original one do). So apply routes hook works after we start live server threads and overrides not available in those threads. For now I change apply_routes decorator to work on class basis. But we also can consider running live server for each individual test method however this will slow things down and inconsistent with django behavior.

  5. We need to write docs.

cc @bittner

@andrewgodwin
Copy link
Member

1: I don't think there's a need for multiple aliases for this one - in fact, raise an error if someone tries to set it.

2: I don't know what it's for either.

3: I believe Twisted will traceback during listen if you try to listen on a port that's already bound. Best to verify this by trying to make it listen on something deliberately already used and checking what happens.

4: If we can make sure Daphne lives in a separate thread but the worker lives in the test thread somehow (though I'm not sure that's possible), the overrides would work. Otherwise, yes, we will need to at least restart the worker for each test.

I also think the test case class should be in a new module, not base.

@bittner
Copy link

bittner commented Jan 24, 2017

I can't comment on the implementation details related to Channels, because I haven't used it yet.

Some comments on the file organization:

  • I agree, I'd move the test case classes into a module called testcases (just like core Django).
  • It's a bit irritating that we have the test infrastructure code and the tests as such in one and the same directory. In core Django this seems more logical: the tests are outside of the django module, and the test infrastructure code lives in django.test -- note the singular.

@proofit404
Copy link
Member Author

@bittner I absolutely agree with you. Its better to keep channel.test as primary package intended for users. We should provide channels.tests for backward compatibility which will show deprecation warning. Also channels own tests should live in the top tests directory and must be excluded from PyPI package. I'll do this change in separate PR if @andrewgodwin agree with this project layout.

@andrewgodwin This is my thoughts so far:

  1. I agree we can make live server support for single channel layer only. If anyone needs multiple channel layers support for real, they will at least come up with some opinion about user api.

  2. and 3. I'll research it at weekend.

  3. Good point, I think it should solve this problem.

  4. All django test cases lives in single module together with LiveServerThread class. This was my inspiration to put channels version into single base module. I don't mind to move it outside.

Thank for feed back guys!

@bittner
Copy link

bittner commented Feb 9, 2017

@andrewgodwin Do you agree with @proofit404's thoughts on the channels/tests/ directory, i.e. moving the actual tests out of the module, and create the test infrastructure in channels/test/, as I suggested?

Is there anything I can do to help this PR move forward?

@proofit404
Copy link
Member Author

@bittner sorry for the huge delay. I can continue working on this PR when I comeback from my business trip on 13th of February.

@andrewgodwin
Copy link
Member

I do think separating out tests and test infrastructure would be a good idea, yes.

* master: (23 commits)
  fix tox (#516)
  Update Binding to support models with UUIDField as primary key (#528)
  Typo in Example using class-based consumer (#526)
  Fix two typos (#521)
  Docs: Python 3.3 is not supported any more (#519)
  Typo in changelog
  Releasing 1.0.3
  Simplify testing infrastructure (#515)
  Fixed #512: Give rundelay a configurable sleep interval
  Add code indent
  Handle slight ordering not being set
  Remove slight ordering from generics docs
  Fixed #509: Docs for enforce_ordering now mirror post-1.0
  Sort imports correctly.
  Remove optional multiplexer arg in generics docs
  Fixed #462: Don't actually close DB connections during tests
  Fixed #505: Add classifiers to setup.py
  Fixed #477: Only re-save session if it's not empty
  Installable benchmark package. (#501)
  Fix typo (#500)
  ...
@proofit404
Copy link
Member Author

Hello guys,

I stuck. Channels test cases use in memory layer implementation. Twisted reactor can be started and stopped only once during whole process live circle. If we use threads for live server, we can't write more than one test case at all. If we use multiprocessing for live server, we can't use in memory layer. >_<

Any suggestions?

@proofit404
Copy link
Member Author

Maybe running behave against real channels installation on developers machine is the best option here.

@andrewgodwin
Copy link
Member

Yes, I think given that limitation it might have to use a cross-process channel backend so we can run the server properly?

@proofit404
Copy link
Member Author

Yes, for live server test case we can substitute project settings with asgi_ipc instead of asgiref.inmemory. If you think this is proper solution of course.

@andrewgodwin
Copy link
Member

I don't want to encourage use of asgi_ipc to be honest, I'm considering deprecating it in favour of something else SQLite-based. I'd rather give people some way to feed in real channel layer settings.

@proofit404
Copy link
Member Author

It is possible to ignore channels test mixin. But that means we can't give certain guaranties about our environment. For example RabbitMQ layer doesn't provide flush extension. In this situation one test can influence into another through messages in the layer.

I can provide test mixin for RabbitMQ which must be used manually in each test case. But this wont be a general solution.

@andrewgodwin
Copy link
Member

Yeah, the flush is kind of important for tests. Maybe the test case could look for the flush extension and then error if it's not present? Both IPC and Redis provide it, so at least there's options.

@proofit404
Copy link
Member Author

Current RabbitmqChannelLayer create new virtual host for each test. This make guaranties that we do not overlap test data even during parallel execution. I can move this in to test helper and make it part of asgi_rabbitmq package. So if user doesn't inherit from this mixin in test, test will fail due to the lack of flush extension. If user inherit from this mixin, test will be executed in the sandbox environment.

Does this sound like a proper solution?

@andrewgodwin
Copy link
Member

I haven't had a chance to look at this yet, it's pretty big and the last couple of weeks have been very busy. I'll try to get to it soon.

Copy link
Member

@andrewgodwin andrewgodwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good overall, I would like to see the conflict resolved and the multiple inheritance removed then I will merge.

# new connection will be established.


class ProcessSetupMixin(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a mixin - both places it is used also inherit from multiprocessing.Process, so you can just make this inherit from that and have single inheritance.

@@ -15,6 +15,9 @@ Major Changes
work but will trigger a deprecation warning, and ``channels.tests`` will be
removed completely in version 1.3.

* Add ``ChannelLiveServerTestCase`` for integration testing. It is
possible to run Selenium with JavaScript which operates WebSockets.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now needs removing from these release notes as we're further along; don't worry about adding release notes, I'll do that when I make the release!

infrastructure is ready default web browser will be also started. You
can open your website in the real browser which can execute JavaScript
and operate on WebSockets. ``live_server_ws_url`` property is also
provided if you decide to run messaging directly from Python.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few grammar and presentation fixes here but I'll fix those post-merge.

@@ -2,5 +2,6 @@


def ws_message(message):
"Echoes messages back to the client"
"""Echoes messages back to the client."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this changed? There's no need for """ unless you're doing multiple lines.

* master:
  Improve docs. (#589)
  Fixed #588: enforce_ordering failed to wait on process-specific chans
  Releasing 1.1.2
  Issue-579: Fix sentence grammar in documentation. (#580)
  Issue-443: Adding session decorators to reference documentation. (#578)
@proofit404
Copy link
Member Author

And we done. Any thoughts on testing extension or asgi_redis test mixin?

@andrewgodwin
Copy link
Member

What do you mean by those?

(also, it looks like master is still failing to test, so this probably will too - I'll fix that and then you can rebase)

@proofit404
Copy link
Member Author

Whole story starts from this comment @kradem provides additional testing and we made some conclusions. Testing extension were proposed to solve shared layer problem between test process and runserver process.

@andrewgodwin
Copy link
Member

Oh, you mean the idea of having a separate test channel layer to run tests on. I'd rather not make this an extension but have people configure a channel layer specifically for tests and pass its alias as an argument to the test class, I think. Trying to make it automatic is probably not possible on all backends, and if you want this level of advanced testing a bit more setup is fine.

* master:
  Remove unused import
  Add explicit checks for asgi library versions
@proofit404
Copy link
Member Author

Hooray test passes.

@kradem
Copy link

kradem commented Apr 7, 2017

Hi @proofit404!

It works very well with the last asgi_rabbitmq, but it shows error in teardown process.

psycopg2.OperationalError: SSL error: sslv3 alert bad record mac ====================================================================== ERROR: test_multichat_functionality (functional_tests.test_channels_page.MultichatFunctionalTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/backends/utils.py", line 62, in execute return self.cursor.execute(sql) psycopg2.OperationalError: SSL error: sslv3 alert bad record mac

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/test/testcases.py", line 216, in call
self._post_teardown()
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/asgi_rabbitmq/test.py", line 63, in _post_teardown
super(RabbitmqLayerTestCaseMixin, self)._post_teardown()
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/channels/test/liveserver.py", line 218, in _post_teardown
super(ChannelLiveServerTestCase, self)._post_teardown()
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/test/testcases.py", line 908, in _post_teardown
self._fixture_teardown()
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/test/testcases.py", line 943, in _fixture_teardown
inhibit_post_migrate=inhibit_post_migrate)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/core/management/init.py", line 130, in call_command
return command.execute(*args, **defaults)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/core/management/base.py", line 345, in execute
output = self.handle(*args, **options)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/core/management/commands/flush.py", line 54, in handle
allow_cascade=allow_cascade)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/core/management/sql.py", line 15, in sql_flush
tables = connection.introspection.django_table_names(only_existing=True, include_views=False)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/backends/base/introspection.py", line 88, in django_table_names
existing_tables = self.table_names(include_views=include_views)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/backends/base/introspection.py", line 56, in table_names
return get_names(cursor)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/backends/base/introspection.py", line 52, in get_names
return sorted(ti.name for ti in self.get_table_list(cursor)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/backends/postgresql/introspection.py", line 66, in get_table_list
AND pg_catalog.pg_table_is_visible(c.oid)""")
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/backends/utils.py", line 64, in execute
return self.cursor.execute(sql, params)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/utils.py", line 94, in exit
six.reraise(dj_exc_type, dj_exc_value, traceback)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/utils/six.py", line 685, in reraise
raise value.with_traceback(tb)
File "/home/balnir/.virtualenvs/multichat/lib/python3.5/site-packages/django/db/backends/utils.py", line 62, in execute
return self.cursor.execute(sql)
django.db.utils.OperationalError: SSL error: sslv3 alert bad record mac



The actual sql variable is "DROP DATABASE 'test_multichat'" command. That database is being dropped at the end of the test suite, but if I duplicate tests the other won't pass:
details test_multichat_functionality (functional_tests.test_channels_page.MultichatFunctionalTest) ... ok ERROR test_multichat_functionality2 (functional_tests.test_channels_page.MultichatFunctionalTest) ... ERROR ERROR

Regards!

@andrewgodwin
Copy link
Member

andrewgodwin commented Apr 10, 2017

I just tried this against a 1.11 installation and it looks like .is_in_memory_db() has gone away - if you merge against master again you will pull in 9266521, which does run the tests against 1.11, but it seems the error is not actually triggered until you try to run a project's tests with the actual test case. Running a test works fine on 1.10, though.

I remember we discussed running integration tests in Travis a while back - did I say no, or was there a technical reason we rejected it?

* master:
  Added a decorator that checks origin headers (#593)
  Added 1.11 to tox (#596)
  Added django 1.11 to travis matrix (#597)
  Actually releasing 1.1.3
  Releasing 1.1.3
  Add start of IRC client spec draft
@proofit404
Copy link
Member Author

Turns out is_in_memory_db call signature was changed in Django 1.11. I already fix this.

Regarding integration tests we decided to exclude them into channel layers. Running this tests requires real layer implementation. Because of that it go against channel policy not use any layer package as a dependency for channels.

I use this live server test cases for integration testing in RabbitMQ layer.

Also there is an open PR in Redis layer. But you can't merge it yet because of this lines before next channels release.

Ideally we need to run layers tests against channels master after each push to it. Probably we can do it with TravisCI API. Running special script in the after_build section should be sophisticated solution.

@andrewgodwin
Copy link
Member

We can definitely get Travis running the layer tests as well after every commit, using the Redis library. I'm going to merge this, and then the things left I'd like one of us to look at are:

  • Having a test alias for the channel layer, and possibly forcing you to define one if you want to use the liveserver test
  • Staticfiles serving support (probably as a class option rather than another subclass)
  • Getting Travis testing integration tests with this as well

@andrewgodwin andrewgodwin merged commit db1b3ba into django:master Apr 11, 2017
@bittner
Copy link

bittner commented Apr 11, 2017

Wahoo! 😲 This is merged, great! 🎉

Thanks @proofit404, thanks thanks thanks!

(Now let's make sure this stay stable and usable for everyone.)

@proofit404
Copy link
Member Author

Hi @andrewgodwin

I'm glad we merge it! Regarding further work on this test case:

  • I think test alias is good idea. This way we need special support only in the layers which doesn't support flush extension.
  • Static files serving support already here. We can add serve_static boolean flag similar to worker_threads class attribute. I think this should be enabled by default. Most people expect this to work out of the box.
  • I prefer to keep integration tests inside layers repositories. I already extend this test case in the rabbitmq layer for advanced logging and profiler support inside child process. Channels definitely doesn't need to know about this internal machinery. Also each layer can have different set of integration tests. Lets see what TravisCI is able to do for non monorepo projects.

First two items of this list already clean enough. I'll implement them.

@proofit404 proofit404 deleted the feature/liveserver-test-case branch April 11, 2017 20:15
@andrewgodwin
Copy link
Member

Ah, yes, I missed the static stuff. We should have a way to turn it on/off easily for sure as people might not want static files on.

@proofit404
Copy link
Member Author

So far we have

chairco pushed a commit to chairco/channels that referenced this pull request Jun 20, 2017
Adds a new test case base class that fires up a live Daphne server and workers for the test case to use, like Django's LiveServerTestCase.
@UrAvgProgrammer
Copy link

Hi @proofit404, I am currently using ChannelsLiveServerTestCase to spin up a runserver for behave, I am using StaticLiveServerTestCase at first but have to change to ChannelsLiveServerTestCase for the websocket support but for some reason now my test for my upload-feature is failing. Does ChannelsLiveServerTestCase support uploading file like StaticLiveServerTestCase?

@proofit404
Copy link
Member Author

Hi @UrAvgProgrammer

I left channels project a long time ago. I can't even recall what all those things have to mean. Sorry 😞

Regards, Artem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants