Re: [tor-dev] Migration of Tor Weather to Stem

Hi,
My name is Sreenatha. I've made an attempt to migrate Tor Weather from TorCtl to Stem as stated in my gsoc proposal(Module 2).
Can you please take a look at the top 3 commits that I've made at my github account? Please suggest any additional changes that you think are necessary.
Cheers, Sreenatha
P.S : I am lucyd@OFTC on IRC.
Hi Sreenatha, glad that you've started in on this! We'd much prefer for development discussions to be on tor-dev@ so looping that in. This looks like a great start. I suspect there is a little confusion though around the socket object that the Controller uses. The Controller doesn't take a socket.socket instance, but rather a stem.socket.ControlSocket... https://stem.torproject.org/api/socket.html This comes up a couple places, most notably... https://github.com/lucyd/weather/commit/ff22e74c247bd61163a974329cd2feb2a121... I doubt that function presently works. Rather, it should be... def listen(): controller = Controller.from_port(port = config.control_port) controller.authenticate(config.authenticator) controller.add_event_listener(newconsensus_listener, EventType.NEWCONSENSUS) I'm not sure what 'config.authenticator' is so that part might be wrong. Also note that this creates a control socket then forgets about it (Weather never cleans it up). This isn't a bug you've introduced but rather what Weather already did. Not the end of the world (interpreter shutdown will clean it up, though possibly with a stacktrace about lingering threads). Have you gotten a Weather instance up and running to test your changes? Cheers! -Damian

Hi Damian, Thanks for pointing out the controller socket error. I've removed my old commits and added new ones at https://github.com/lucyd/weather/commits/master. Can you please take a look at them? I haven't got a weather instance up but I followed the instructions in the README to test it and 2 of the 9 tests failed. ====================================================================== FAIL: Test a subscribe attempt to all subscription types, relying ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/lucyd/Desktop/weather/weather/../weather/weatherapp/tests.py", line 311, in test_subscribe_all self.assertEqual(node_down_sub.grace_pd, 1) AssertionError: 0 != 1 ====================================================================== FAIL: Test a node down subscription (all other subscriptions off) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/lucyd/Desktop/weather/weather/../weather/weatherapp/tests.py", line 89, in test_subscribe_node_down self.assertEqual(node_down_sub.grace_pd, 1) AssertionError: 0 != 1 ---------------------------------------------------------------------- It seems that an incorrect assert is causing the tests to fail. self.assertEqual(node_down_sub.grace_pd, 1) I looked it up and it appears that *grace_pd* is the default number of hours which the subscriber's router must be offline before a notification is sent and it's default value is zero and since it isn't set to any value in the test, it assumed it's default value and the tests failed. I modified the asserts as below and all the tests passed. self.assertEqual(node_down_sub.grace_pd, 0) Am I missing something? Or, are those tests incorrect? Also, should I have a weather instance up and running and check manually for email notifications to subscriptions or will the above tests suffice? Cheers, Sreenatha On Sun, Jun 16, 2013 at 2:06 AM, Damian Johnson <atagar@torproject.org>wrote:
Hi,
My name is Sreenatha. I've made an attempt to migrate Tor Weather from TorCtl to Stem as stated in my gsoc proposal(Module 2).
Can you please take a look at the top 3 commits that I've made at my github account? Please suggest any additional changes that you think are necessary.
Cheers, Sreenatha
P.S : I am lucyd@OFTC on IRC.
Hi Sreenatha, glad that you've started in on this! We'd much prefer for development discussions to be on tor-dev@ so looping that in.
This looks like a great start. I suspect there is a little confusion though around the socket object that the Controller uses. The Controller doesn't take a socket.socket instance, but rather a stem.socket.ControlSocket...
https://stem.torproject.org/api/socket.html
This comes up a couple places, most notably...
https://github.com/lucyd/weather/commit/ff22e74c247bd61163a974329cd2feb2a121...
I doubt that function presently works. Rather, it should be...
def listen(): controller = Controller.from_port(port = config.control_port) controller.authenticate(config.authenticator) controller.add_event_listener(newconsensus_listener, EventType.NEWCONSENSUS)
I'm not sure what 'config.authenticator' is so that part might be wrong. Also note that this creates a control socket then forgets about it (Weather never cleans it up). This isn't a bug you've introduced but rather what Weather already did. Not the end of the world (interpreter shutdown will clean it up, though possibly with a stacktrace about lingering threads).
Have you gotten a Weather instance up and running to test your changes?
Cheers! -Damian

Hi Sreenatha, thanks for getting this project going! I've pushed a series of commits to further clean up weather's ctlutil.py module... https://gitweb.torproject.org/user/atagar/weather.git I'm not sure about testing this service though. Christian will know best how we should proceed. On Fri, Jul 5, 2013 at 1:40 AM, Sreenatha Bhatlapenumarthi <sreenatha.dev@gmail.com> wrote:
Hi Damian,
Thanks for pointing out the controller socket error. I've removed my old commits and added new ones at https://github.com/lucyd/weather/commits/master. Can you please take a look at them?
I haven't got a weather instance up but I followed the instructions in the README to test it and 2 of the 9 tests failed.
====================================================================== FAIL: Test a subscribe attempt to all subscription types, relying ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/lucyd/Desktop/weather/weather/../weather/weatherapp/tests.py", line 311, in test_subscribe_all self.assertEqual(node_down_sub.grace_pd, 1) AssertionError: 0 != 1
====================================================================== FAIL: Test a node down subscription (all other subscriptions off) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/lucyd/Desktop/weather/weather/../weather/weatherapp/tests.py", line 89, in test_subscribe_node_down self.assertEqual(node_down_sub.grace_pd, 1) AssertionError: 0 != 1
----------------------------------------------------------------------
It seems that an incorrect assert is causing the tests to fail.
self.assertEqual(node_down_sub.grace_pd, 1)
I looked it up and it appears that grace_pd is the default number of hours which the subscriber's router must be offline before a notification is sent and it's default value is zero and since it isn't set to any value in the test, it assumed it's default value and the tests failed. I modified the asserts as below and all the tests passed.
self.assertEqual(node_down_sub.grace_pd, 0)
Am I missing something? Or, are those tests incorrect? Also, should I have a weather instance up and running and check manually for email notifications to subscriptions or will the above tests suffice?
Cheers, Sreenatha
On Sun, Jun 16, 2013 at 2:06 AM, Damian Johnson <atagar@torproject.org> wrote:
Hi,
My name is Sreenatha. I've made an attempt to migrate Tor Weather from TorCtl to Stem as stated in my gsoc proposal(Module 2).
Can you please take a look at the top 3 commits that I've made at my github account? Please suggest any additional changes that you think are necessary.
Cheers, Sreenatha
P.S : I am lucyd@OFTC on IRC.
Hi Sreenatha, glad that you've started in on this! We'd much prefer for development discussions to be on tor-dev@ so looping that in.
This looks like a great start. I suspect there is a little confusion though around the socket object that the Controller uses. The Controller doesn't take a socket.socket instance, but rather a stem.socket.ControlSocket...
https://stem.torproject.org/api/socket.html
This comes up a couple places, most notably...
https://github.com/lucyd/weather/commit/ff22e74c247bd61163a974329cd2feb2a121...
I doubt that function presently works. Rather, it should be...
def listen(): controller = Controller.from_port(port = config.control_port) controller.authenticate(config.authenticator) controller.add_event_listener(newconsensus_listener, EventType.NEWCONSENSUS)
I'm not sure what 'config.authenticator' is so that part might be wrong. Also note that this creates a control socket then forgets about it (Weather never cleans it up). This isn't a bug you've introduced but rather what Weather already did. Not the end of the world (interpreter shutdown will clean it up, though possibly with a stacktrace about lingering threads).
Have you gotten a Weather instance up and running to test your changes?
Cheers! -Damian
participants (2)
-
Damian Johnson
-
Sreenatha Bhatlapenumarthi