Unit tests for pydy/viz/server.py #325

kdexd · 2016-02-02T11:35:19Z

This PR is in correspondence to issue server.py needs unit tests #243 and contains work on a newly added file under the directory structure as : pydy/viz/tests/test_server.py
These tests are for the file : pydy/viz/server.py

Makes a new class `TestStoppableHTTPServer`. - Adds method test_stoppable_http_server which tests whether the StoppableHttpServer binds and stops properly. - Test is passed when the boolean `run` of StoppableHTTPServer toggles True/ False on binding/ stopping the server.

kdexd · 2016-02-02T11:37:40Z

@moorepants I will keep on adding more tests incrementally. Currently, I wrote tests for the StoppableHTTPServer class in server.py.Please view the commit message and the code, and suggest me improvements as I am new to test driven development procedure.

oliverlee · 2016-02-02T13:26:00Z

pydy/viz/tests/test_server.py

+
+    def test_stoppable_http_server(self):
+        self.stoppable_http_server.server_bind()
+        assert self.stoppable_http_server.run is True


Avoid comparing boolean values to True or False using == or is.
https://www.python.org/dev/peps/pep-0008/#programming-recommendations

Ok, I'll fix this.

moorepants · 2016-02-02T17:31:59Z

If you need some sample data for a scene check out the tests for the scene class. It generates some base data.

- self.http_server declaration was not used anywhere so removed - boolean comparison using `is` refactored

kdexd · 2016-02-03T06:24:01Z

Well, I was able to generate two JSON files, one containing scene data, other containing visualization data. Only the JSON file containing scene data is of my interest for server class testing.
The files are generated through scene tests, so they cannot be generated and deleted in the server tests, as if the scene tests fail by chance, the server tests will also fail -- though generating JSON was not the task of server.
I am thinking of keeping a sample JSON file permanently under the static directory exclusively for server tests, this way the tests will stay independent of each other.
Is this way appropriate? Please suggest better workarounds if any..

kdexd · 2016-02-03T10:52:53Z

About my previous comment, I need not generate JSON files for the purpose, I found them under static/js/tests/sample_data directory.

oliverlee · 2016-02-03T11:51:57Z

For future reference, the output of one test should never be dependent on another. It's better to generate data at the beginning of the test or to add test data files.

- The method `test_run_server` is broken and needs fix.

kdexd · 2016-02-03T13:11:57Z

I can't find a suitable way to stop the server. Help needed here. The test here won't pass in this commit.

oliverlee · 2016-02-03T21:00:50Z

You can click on the red x to look at the test failure details. You can then look at the console output for each job to see why your test is failing.

The test fails before the server starts running.

kdexd · 2016-02-04T03:46:27Z

Strange.. I gives as OSError, I didn't face it when I ran nosetests on my machine.
I will have to change directory of Server class from static/ to ../static/ to overcome this error.
Also, running the server opens the browser by default, which should be separated out in my opinion, as a person might want to open the browser later. We are anyways printing in the command line, that visit this URL from your browser..

moorepants · 2016-02-04T05:05:40Z

IPython opens the server by default. I think we were copying that functionality here.

kdexd · 2016-02-04T05:41:53Z

Shifting the line webbrowser.open(url) to a separate method does not open the server.

Also I need to perform assertions and further shutdown the server by sending it SIGINT and a "Y" as stdin. But on running the tests, the server starts and nothing can be performed further.

I tried to use subprocess, in the previous commit, but the method subprocess.communicate waits for the process to end.

On manually stopping the server, I get an EOFError, due to killing of the subprocess and this test fails.

What should I do for it?

oliverlee · 2016-02-04T09:52:46Z

You call communicate() twice, the first with no input. As it has no input, the first call will never terminate.

Call communicate() once and with input and then use wait() with a timeout.

kdexd · 2016-02-04T09:56:25Z

@oliverlee This trick doesn't work as the test does not move on to next line of execution once the server is started.

kdexd · 2016-02-04T12:08:51Z

@oliverlee @moorepants Hi, while testing run_server, the moment I execute it, the server starts and the test does not move further as it waits for the server to stop. I have tried multiprocessing.Process, multiprocessing.Pool, subprocess, thread, threading.

None seem to just open the server in the background and let me programatically generate an interrupt and further provide a "y" as stdin, to shutdown the server.

I thought the thread or threading modules will surely help, but the method contains a signal.signal, which is only valid in main thread.

What should I do here now ?

oliverlee · 2016-02-04T12:51:29Z

If you comment out the code, I cannot see how the test fails on Travis.

oliverlee · 2016-02-04T16:31:51Z

The subprocess/Popen module is used to spawn new processes and is intended to replace several older modules and functions, namely os.system. https://docs.python.org/3/library/subprocess.html

We want to run the server in another thread as the current behavior blocks. I've submitted #326 to move execution into a background thread allowing you to stop it easily.

Here's an example:

    def test_run_server(self):
        self.test_server.run_server(headless=True)
        assert self.test_server.httpd.running

        sys.stdin = io.StringIO('y')
        os.kill(os.getpid(), signal.SIGINT)
        assert not self.test_server.httpd.running
        assert not self.test_server._thread.is_alive()

oliverlee · 2016-02-04T16:34:46Z

I don't know what directory you're running the tests in, but it fails on travis:

======================================================================
ERROR: pydy.viz.tests.test_server.TestServer.test_run_server
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/test-env/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/pydy/pydy/pydy/viz/tests/test_server.py", line 41, in test_run_server
    proc = subprocess.Popen(self.test_server.run_server(),
  File "/home/travis/build/pydy/pydy/pydy/viz/server.py", line 81, in run_server
    os.chdir(self.directory)
OSError: [Errno 2] No such file or directory: '../static/'

You need to set the directory argument of Server during initialization.

kdexd · 2016-02-04T16:41:46Z

@oliverlee That is because after one test, the directory cannot be changed to static/ again as we get into static/ for the first time.

This happened on my machine too. I executed os.chdir("..") to get out of the static/ directory after doing the tests.

Does travis test in the same container as appveyor ? It seems strange as I could see one test passing and another failing afterwards sometime ago.

oliverlee · 2016-02-04T16:48:27Z

The directory argument is not set correctly because the working directory on travis is likely different than what you are using. Something like this will set the correct directory regardless of your working directory:

os.path.join(os.path.dirname(__file__), '../static')

However, I don't know if the server will modify files in place. Maybe it would be better to create a new directory for each test? @moorepants @sahilshekhawat

I have no idea what appveyor does but it tests Windows whereas travis tests Linux.

kdexd · 2016-02-04T16:51:48Z

Ok, so once your PR is merged, I will pull the code and try to complete these tests.

oliverlee · 2016-02-04T16:52:47Z

You can merge the PR locally and work on your tests before it gets merged into master.

…ead' into tests_server

kdexd · 2016-02-05T14:10:10Z

@oliverlee The tests will pass only if I press the enter key manually while nosetests is being executed. This is why the tests passed locally but failed on appveyor and travis, due to absence of manual intervention.

I need to provide an EOF probably.

kdexd · 2016-02-05T19:44:42Z

Ok, so Travis CI builds are passing, but Appveyor are failing. Why so ? I can't see the reason, only show that the process exited with status 1.

moorepants · 2016-02-06T15:37:26Z

This is the appveyor line that sow failure:

pydy.viz.tests.test_server.TestServer.test_run_server ... Command exited with code 1

Appveyor runs the code on windows. You have to make sure to use cross platform python commands, e.g. not os.system() or other things like that.

kdexd · 2016-02-06T15:42:20Z

It isn't actually showing the exact error, I do not have Windows distro on my laptop, only Ubuntu. Though i suspect it is os.chdir that might be troubling.

moorepants · 2016-02-06T15:59:02Z

os.chdir should work on Windows. There are a few Python commands that have trouble on Windows, for example shutil.rmtree.

yashu-seth · 2016-02-06T19:29:37Z

@karandesai-96 Have you ensured that the changes are consistent with pep8? I remember I got the same error and got rid of it only after fixing pep8 issues.

moorepants · 2016-02-06T20:35:38Z

The minimal pep8 checks we have only run on the Travis system, not appveyor.

yashu-seth · 2016-02-06T20:38:33Z

Ok. Thanks.

moorepants · 2016-03-08T19:28:18Z

FYI, #326 has been merged. Maybe that will help. @oliverlee said that you can't check this on appveyor though, due to some limitation. Maybe we should only have the test that sends the kill signal run on local machines.

kdexd · 2016-03-09T02:27:23Z

Sounds like a plan 😄

oliverlee reviewed Feb 2, 2016
View reviewed changes

Adds python3 compatibility, removes redundant declaration

a562ab9

- self.http_server declaration was not used anywhere so removed - boolean comparison using `is` refactored

Adds TestServer class to test Server.

ebb4985

- The method `test_run_server` is broken and needs fix.

Temporarily comment out parts which are failing tests.

949ebf6

karandesai-96 and others added 3 commits February 4, 2016 19:26

Use subprocess.Popen to handle run_server.

23bec7c

Add headless option to server for testing

61dd0db

Run viz webserver in a background thread

ed9a927

karandesai-96 added 2 commits February 4, 2016 22:43

Merge remote-tracking branch 'remotes/oliver/vizserver-background-thr…

918ad0d

…ead' into tests_server

TestServer class now complete with test_run_server.

ec75265

kdexd mentioned this pull request Feb 11, 2016

Test sigint travis #328

Closed

Provide sys.stdin at correct time in test_server.py

5ee0875

kdexd closed this Jul 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests for pydy/viz/server.py #325

Unit tests for pydy/viz/server.py #325

kdexd commented Feb 2, 2016

kdexd commented Feb 2, 2016

oliverlee Feb 2, 2016

kdexd Feb 2, 2016

moorepants commented Feb 2, 2016

kdexd commented Feb 3, 2016

kdexd commented Feb 3, 2016

oliverlee commented Feb 3, 2016

kdexd commented Feb 3, 2016

oliverlee commented Feb 3, 2016

kdexd commented Feb 4, 2016

moorepants commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

oliverlee commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 5, 2016

kdexd commented Feb 5, 2016

moorepants commented Feb 6, 2016

kdexd commented Feb 6, 2016

moorepants commented Feb 6, 2016

yashu-seth commented Feb 6, 2016

moorepants commented Feb 6, 2016

yashu-seth commented Feb 6, 2016

moorepants commented Mar 8, 2016

kdexd commented Mar 9, 2016

Unit tests for pydy/viz/server.py #325

Unit tests for pydy/viz/server.py #325

Conversation

kdexd commented Feb 2, 2016

kdexd commented Feb 2, 2016

oliverlee Feb 2, 2016

Choose a reason for hiding this comment

kdexd Feb 2, 2016

Choose a reason for hiding this comment

moorepants commented Feb 2, 2016

kdexd commented Feb 3, 2016

kdexd commented Feb 3, 2016

oliverlee commented Feb 3, 2016

kdexd commented Feb 3, 2016

oliverlee commented Feb 3, 2016

kdexd commented Feb 4, 2016

moorepants commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

oliverlee commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 4, 2016

oliverlee commented Feb 4, 2016

kdexd commented Feb 5, 2016

kdexd commented Feb 5, 2016

moorepants commented Feb 6, 2016

kdexd commented Feb 6, 2016

moorepants commented Feb 6, 2016

yashu-seth commented Feb 6, 2016

moorepants commented Feb 6, 2016

yashu-seth commented Feb 6, 2016

moorepants commented Mar 8, 2016

kdexd commented Mar 9, 2016