Rover self register by hbradio · Pull Request #91 · rovercode/rovercode

hbradio · 2017-02-24T16:19:53Z

Makes rover periodically check in with rovercode-web.

Also, fixes test coverage when in Docker.

coveralls · 2017-02-24T16:22:36Z

Coverage decreased (-27.3%) to 43.5% when pulling 82daf61 on feature/Rover_self_register into 5c76555 on development.

hbradio · 2017-02-24T17:26:51Z

www/app.py

+    print "rovercode-web id is " + str(web_id)
+    return r
+
+def heartbeat_thread():


I'm open to ideas about how to unit test this function.

coveralls · 2017-02-24T18:28:41Z

Coverage decreased (-27.3%) to 43.5% when pulling c705b6f on feature/Rover_self_register into eaf939d on development.

cabarnes · 2017-02-24T18:23:10Z

README.md

 $ py.test
 ```
-We're having some trouble running pytest-cov inside the Docker container. If you are using Docker for development and want to run test, you'll need to `pip install pytest pytest-cov` outside and run the `py.test` outside of Docker for now.
+Or, if your using Docker, make sure the container is running (the `sudo docker run ...` command above), then do:


cabarnes · 2017-02-24T18:25:10Z

www/app.py

+def register_with_web():
+    global web_id
+    global payload
+    payload = {'name': 'Chipy', 'owner': 'Mr. Hurlburt', 'local_ip': get_local_ip()}


The name and owner will eventually come from some kind of configuration?

cabarnes · 2017-02-24T18:45:59Z

www/app.py

+    global payload
+    payload = {'name': 'Chipy', 'owner': 'Mr. Hurlburt', 'local_ip': get_local_ip()}
+    print "Registering with rovercode-web"
+    r = requests.post("https://rovercode.com/mission-control/rovers/", payload)


I would suggest moving the base URL somewhere that can be accessed by everything needing it so you only will have to change it in one place in the future.

Good call. Will do.

cabarnes · 2017-02-24T18:52:20Z

www/tests/test_app.py

+def test_sensor_init():
    assert len(app.binary_sensors) > 0
+
+def test_get_local_ip():


To test all of these effectively, you'll need to use something like responses. It's a way to mock up the response from the server so that you don't actually need a server for the tests. Then you can check through all of the error cases as well. I can help you with it at some point if you run into issues. Basically, you setup a response to a particular request and see how the function behaves.

Yeah, I'll try it out!
The other challenge with heartbeat_thread is that it is an infinite loop. Any ideas on how to get around that problem?

I think you'll need a way to exit the loop. A shutdown variable or something like it that it's checking every time through the loop. If it were part of a class, you could have a class method to start and stop the thread. I'm not sure if the socketio background task will accept a class member or not.

I wrapped it in a class with a stop function that disabled the shutdown variable. And socketio has not problem accepting a class member.

But, in my test function, how do I get the result of the thread function? If I start it in the background using socketio, I don't know where the return goes to. If I start it in the foreground, it blocks and I can't call the .stopThread function.

Maybe a better way is to add an optional parameter (run_once) that is false by default, but that the test can set to true so that it just runs through once. That won't give 100% test coverage, but it will test most of it. I think you'd have to spin up a task in your test in order to stop the task in the infinite loop case.

Sounds good -- I'll go with the run_once strategy for now.

cabarnes · 2017-02-24T18:52:43Z

www/app.py

+    print "rovercode-web id is " + str(web_id)
+    return r
+
+def heartbeat_thread():


…age for testing

…rnetof/rovercode into feature/Rover_self_register

This reverts commit f9514a1.

coveralls · 2017-03-04T18:01:47Z

Coverage decreased (-12.7%) to 58.011% when pulling c5f0144 on feature/Rover_self_register into eaf939d on development.

hbradio · 2017-03-04T18:02:39Z

Also added prospector and fixed the lint warning.

coveralls · 2017-03-04T18:04:52Z

Coverage decreased (-12.7%) to 58.011% when pulling c5f0144 on feature/Rover_self_register into eaf939d on development.

cabarnes · 2017-03-05T01:46:02Z

.travis.yml

  - pip install coveralls
 script:
  - py.test
+  - pwd && prospector


Do you still need to print the directory here or was that for debugging?

Nope, I'll remove it!

cabarnes · 2017-03-05T01:49:19Z

www/app.py

-print "Registering rover!"
+ws_thread = None
+hb_thread = None
+payload = None;


Whoops, will fix

cabarnes · 2017-03-05T02:11:58Z

www/tests/test_app.py

+                  json=response_payload, status=200,
+                  content_type='application/json')
+    result = heartbeat_manager.register()
+    assert result.status_code in [200, 201]


You set the response to 200 so you should assert that it is exactly 200.

Good point. Will change.

cabarnes · 2017-03-05T02:14:06Z

www/tests/test_app.py

+    web_id = 333
+    heartbeat_manager = app.HeartBeatManager(payload=payload)
+    response_payload = payload.copy()
+    response_payload['id'] = 333


You can use the web_id variable here.

Good catch, will do.

cabarnes · 2017-03-05T02:19:58Z

www/app.py

+payload = None;

 pwm = pwmLib.get_platform_pwm(pwmtype="softpwm")
 gpio = gpioLib.get_platform_gpio();


Whoops, will fix all of these.

cabarnes · 2017-03-05T02:23:21Z

www/app.py

+        self.run = True;
+        self.thread = None;
+        self.web_id = id;
+        self.payload = payload;


Stray ; for each of these

cabarnes · 2017-03-05T02:36:10Z

www/tests/test_app.py

+                  content_type='application/json')
+
+    result = heartbeat_manager.thread_func(run_once=True)
+    assert result.status_code in [200, 201]


cabarnes · 2017-03-05T02:37:24Z

www/tests/test_app.py

+    assert len(responses.calls) == 1
+    assert responses.calls[0].request.url == app.ROVERCODE_WEB_REG_URL+str(web_id)+"/"
+
+    # Test for a rover who has been forgotten by the server


This should be a separate test

Good point. I'll separate it out.

cabarnes · 2017-03-05T02:38:09Z

www/tests/test_app.py

+
+    heartbeat_manager.web_id = web_id_new
+    result = heartbeat_manager.thread_func(run_once=True)
+    assert result.status_code in [200, 201]


cabarnes · 2017-03-05T02:49:39Z

Looks like most of the missing test coverage is in the API handlers for the stored block diagrams. All of that will be going away since they will be stored in the cloud right?

cabarnes · 2017-03-05T02:51:52Z

If you hadn't already found it, there's a visual representation of the test coverage output in htmlcov/index.html.

hbradio · 2017-03-05T03:28:04Z

I hadn't found the visual representation. That's awesome.

hbradio · 2017-03-05T13:39:13Z

Agreed -- a lot of the uncovered code deals with saving block diagrams. We can delete that stuff. Maybe in a separate PR?

coveralls · 2017-03-05T13:52:18Z

Coverage decreased (-12.7%) to 58.011% when pulling 84c5dd6 on feature/Rover_self_register into eaf939d on development.

cabarnes · 2017-03-05T21:38:42Z

Separate PR is fine with me. There is one missing change for asserting 200 from my comments above and then I can approve.

coveralls · 2017-03-06T20:25:06Z

Coverage decreased (-12.7%) to 58.011% when pulling 8050e90 on feature/Rover_self_register into eaf939d on development.

hbradio added 3 commits February 22, 2017 23:11

Makes rover periodically check in. Uses PUT to rover id

bf372b9

Adds tests for web register. Fixes py.test in docker

395e3c4

Descreases check-in time to 3 seconds

82daf61

hbradio requested a review from cabarnes February 24, 2017 16:19

Merge branch 'development' into feature/Rover_self_register

c705b6f

hbradio commented Feb 24, 2017

View reviewed changes

cabarnes suggested changes Feb 24, 2017

View reviewed changes

hbradio added 15 commits March 3, 2017 15:15

Restructures HeartbeatManager for better testing. Adds Responses pack…

15dbb58

…age for testing

Improves test coverage of heartbeat. Trims things out of coverage

5763220

Removes unneeded separate script for registering rover

230219f

Adds prospector for linting

b33928d

Fixes lint errors

1624bbd

Merge branch 'feature/Rover_self_register' of ssh://github.com/aninte…

18f7200

…rnetof/rovercode into feature/Rover_self_register

Adds new dependencies to travis.yml

02f5987

Fixes version of prospector

68c82b0

Makes prospector run in the correct directory on Travis

f9514a1

Revert "Makes prospector run in the correct directory on Travis"

d1f8c0e

This reverts commit f9514a1.

Adds .prospector.yaml

2ef2c96

Adds travis debug to see where it is running prospector from

aa124cc

Changes location of Adafruit lib on travis build to match normal setup

08b02c7

Install existing Adafruit lib instance instead of cloning new one

cdd1301

Fixes lint errors with API docstrings

c5f0144

hbradio added 3 commits March 4, 2017 18:12

Cleans some style and uses a variable better

95f9462

Separates heartbeat test into two

93acbac

Fixes docstring lint error

84c5dd6

cabarnes suggested changes Mar 5, 2017

View reviewed changes

Requires that request responds with 200 instead of 201

8050e90

cabarnes approved these changes Mar 6, 2017

View reviewed changes

hbradio merged commit 72b90b2 into development Mar 6, 2017

hbradio deleted the feature/Rover_self_register branch March 6, 2017 20:26

Conversation

hbradio commented Feb 24, 2017

Uh oh!

coveralls commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 4, 2017

Uh oh!

hbradio commented Mar 4, 2017

Uh oh!

coveralls commented Mar 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cabarnes commented Mar 5, 2017

Uh oh!

cabarnes commented Mar 5, 2017

Uh oh!

hbradio commented Mar 5, 2017

Uh oh!

hbradio commented Mar 5, 2017

Uh oh!

coveralls commented Mar 5, 2017

Uh oh!

cabarnes commented Mar 5, 2017