Hi Damian,
Looking at coverage the test provides, I was under the impression that the various cases of include_fields and exclude fields (e.g. if they overlap) should also be tested, but if you find that unnecessary, that would help explain why my unit tests were difficult to follow.
In the mocking changes that I made, I added functionality for mocking functions that take keyword arguments. Though it is no longer necessary with the new unit tests, I was wondering if there was something I could have done better with that code as it seems such functionality may come in useful in the future.
You may have already noticed this (and your documentation is consistent with this), but just to be sure, the new export code won't work with generators, which I bring up as this is what server_descriptor.parse_file() returns. So a user of this module would need to covert that generator into a list or tuple, which could be rather large.
def gener(list=[1,2,3,4]):
... for item in list: ... yield item
gener()[0]
Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: 'generator' object has no attribute '__getitem__'
Working with Professor Danner this summer (and from the CS classes I have taken so far), I am under the impression that run-time type checking can be expensive and generally falls outside of the realm of 'pythonic' code. To help Megan and I understand this design change, could you explain why you went with this style (e.g. line 83 in stem/descriptor/export.py)? Along similar lines, why wouldn't we also check the types of other parameters passed into the function?
Thank you in advance for answering these questions -- this summer has been (and still is) a phenomenal learning experience for us, and gaining insight on coding style and the like is crucial to developing our own coding techniques.
Looking at the code, I must apologize. I didn't mean to take so much of your time rewriting the code. That said, looking at your implementation gave me some perspective in evaluating the code I submitted so I don't make similar mistakes in the future.
Best, Erik
On Sat, Aug 4, 2012 at 3:22 PM, Tor Bug Tracker & Wiki < torproject-admin@torproject.org> wrote:
#6512: TorExport Module and Unit Testing
----------------------------+----------------------------------------------- Reporter: ErikI | Owner: atagar Type: enhancement | Status: closed Priority: normal | Milestone: Component: Stem | Version: Resolution: implemented | Keywords: Parent: | Points: Actualpoints: |
----------------------------+----------------------------------------------- Changes (by atagar):
- status: needs_review => closed
- resolution: => implemented
Comment:
Thanks Erik, merged with some revisions [1]. I found the unit tests a bit hard to follow, especially since it wasn't broken down by the test cases so I rewrote those. Let me know if I missed any test cases that you think are important.
[1]
https://gitweb.torproject.org/stem.git/commitdiff/7022021c4207a0066cb9326162...
-- Ticket URL: < https://trac.torproject.org/projects/tor/ticket/6512#comment:3%3E Tor Bug Tracker & Wiki https://trac.torproject.org/ The Tor Project: anonymity online