Hi Damian,<div><br></div><div>Thanks for pointing out the controller socket error.</div><div>I've removed my old commits and added new ones at <a href="https://github.com/lucyd/weather/commits/master">https://github.com/lucyd/weather/commits/master</a>.</div>
<div>Can you please take a look at them?</div><div><br></div><div>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. </div><div><br></div><div><div>
<font face="courier new, monospace">======================================================================</font></div><div><font face="courier new, monospace">FAIL: Test a subscribe attempt to all subscription types, relying</font></div>
<div><font face="courier new, monospace">----------------------------------------------------------------------</font></div><div><font face="courier new, monospace">Traceback (most recent call last):</font></div><div><font face="courier new, monospace"> File "/home/lucyd/Desktop/weather/weather/../weather/weatherapp/tests.py", line 311, in test_subscribe_all</font></div>
<div><font face="courier new, monospace"> self.assertEqual(node_down_sub.grace_pd, 1)</font></div><div><font face="courier new, monospace">AssertionError: 0 != 1</font></div><div><font face="courier new, monospace"><br>
</font></div><div><font face="courier new, monospace">======================================================================</font></div><div><font face="courier new, monospace">FAIL: Test a node down subscription (all other subscriptions off)</font></div>
<div><font face="courier new, monospace">----------------------------------------------------------------------</font></div><div><font face="courier new, monospace">Traceback (most recent call last):</font></div><div><font face="courier new, monospace"> File "/home/lucyd/Desktop/weather/weather/../weather/weatherapp/tests.py", line 89, in test_subscribe_node_down</font></div>
<div><font face="courier new, monospace"> self.assertEqual(node_down_sub.grace_pd, 1)</font></div><div><font face="courier new, monospace">AssertionError: 0 != 1</font></div><div><font face="courier new, monospace"><br>
</font></div><div><font face="courier new, monospace">----------------------------------------------------------------------</font></div></div><div><br></div><div>It seems that an incorrect assert is causing the tests to fail.</div>
<div><br></div><div><font face="courier new, monospace">self.assertEqual(node_down_sub.grace_pd, 1)</font> </div><div> </div><div>I looked it up and it appears that <i>grace_pd</i> 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.</div>
<div><br></div><div><div><font face="courier new, monospace">self.assertEqual(node_down_sub.grace_pd, 0)</font> </div></div><div><br></div><div>Am I missing something? Or, are those tests incorrect?</div><div>Also, should I have a weather instance up and running and check manually for email notifications to subscriptions or will the above tests suffice?</div>
<div><br></div><div>Cheers,</div><div>Sreenatha</div><div> <br><br><div class="gmail_quote">On Sun, Jun 16, 2013 at 2:06 AM, Damian Johnson <span dir="ltr"><<a href="mailto:atagar@torproject.org" target="_blank">atagar@torproject.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">> Hi,<br>
><br>
> My name is Sreenatha. I've made an attempt to migrate Tor Weather<br>
> from TorCtl to Stem as stated in my gsoc proposal(Module 2).<br>
><br>
> Can you please take a look at the top 3 commits that I've made at my<br>
> github account?<br>
> Please suggest any additional changes that you think are necessary.<br>
><br>
> Cheers,<br>
> Sreenatha<br>
><br>
> P.S : I am lucyd@OFTC on IRC.<br>
<br>
</div></div>Hi Sreenatha, glad that you've started in on this! We'd much prefer<br>
for development discussions to be on tor-dev@ so looping that in.<br>
<br>
This looks like a great start. I suspect there is a little confusion<br>
though around the socket object that the Controller uses. The<br>
Controller doesn't take a socket.socket instance, but rather a<br>
stem.socket.ControlSocket...<br>
<br>
<a href="https://stem.torproject.org/api/socket.html" target="_blank">https://stem.torproject.org/api/socket.html</a><br>
<br>
This comes up a couple places, most notably...<br>
<br>
<a href="https://github.com/lucyd/weather/commit/ff22e74c247bd61163a974329cd2feb2a121db94#L0R26" target="_blank">https://github.com/lucyd/weather/commit/ff22e74c247bd61163a974329cd2feb2a121db94#L0R26</a><br>
<br>
I doubt that function presently works. Rather, it should be...<br>
<br>
def listen():<br>
controller = Controller.from_port(port = config.control_port)<br>
controller.authenticate(config.authenticator)<br>
controller.add_event_listener(newconsensus_listener, EventType.NEWCONSENSUS)<br>
<br>
I'm not sure what 'config.authenticator' is so that part might be<br>
wrong. Also note that this creates a control socket then forgets about<br>
it (Weather never cleans it up). This isn't a bug you've introduced<br>
but rather what Weather already did. Not the end of the world<br>
(interpreter shutdown will clean it up, though possibly with a<br>
stacktrace about lingering threads).<br>
<br>
Have you gotten a Weather instance up and running to test your changes?<br>
<br>
Cheers! -Damian<br>
</blockquote></div><br></div>