[tor-dev] Migration of Tor Weather to Stem

[Damian Johnson]

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 at 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 at 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 at 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/ff22e74c247bd61163a974329cd2feb2a121db94#L0R26
>>
>> 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
>
>