[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-dev] Stem code review 2012-12-04
Stem devs,
I did a code review of recent Stem commits ("Tor event handling" merge (commit 42872dd08e81d6b3) through "Checking for None by identity" (commit 69f72efc9367092c)). My comments and questions follow. I will skip my own contributions (they were great 8-) in that range, as I am biased.
Event._log_if_unrecognized() is a good idea that vastly improves the readability of Event subclasses and reduces code duplication.
Regarding commit fb0aec5d95e9d2e6 "tidying up boilerplate":
1) I do not like the new _get_event() with assert_class and assert_content. There are transformations and tests and returned values all within what is a mock object builder, meaning it works via side-effect. This could be surprising to test writers.
2) I vote to keep "self.assertTrue(isinstance(event, stem.response.events.StatusEvent))" style tests after producing the event.
The quoted key/value mapping is more readable, now. Good work. Why not look for quoted positional args before non-quoted positional args? Why not do just like kwarg handling?
Why restrict SignalEvents to expected_signals when control-spec.txt allows more? This may mean changes later to add support for things the protocol already claims to support.
I set up coverage.py for another project and I wondered if it would work with Stem. So, I ran "coverage run --parallel-mode --branch --omit="test*" ./run_tests.py -u -i -t RUN_NONE" in the stem directory. The results are 66% - 100% coverage per module. Another impressive accomplishment. And this is only running a subset of the possible tests.
--
Sean Robinson
_______________________________________________
tor-dev mailing list
tor-dev@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev