The changes look great! I see no issues with merging to the master branch.
Great, pushed.
Sorry about the reStructuredText inconvenience. I actually didn't even think of it -- I just meant to clean up the comments a bit, but I'll definitely be more careful in the future.
No worries. Again, it would usually have been a welcome change. :)
Once we confirm this with Damian, he will push the changes to master in torprojects' repository.
Don't worry about that. The way that open source git projects usually work is that you make a 'pull request', which simply means saying 'I have some changes that I would like to share, here they are' (which is what you've been doing). If the changes look good then I'll rebase them onto the master branch and push them myself.
Usually only a tiny number of central developers actually have push access to the master repository.
Our code can be found at:
https://github.com/meganchang/Stem/blob/proc-tests/test/unit/util/proc.py
Ok. Previously I was pulling from 'jacthinman', is 'meganchang' the github repository where you plan to do most of your future work?
I'd suggest periodically running the following, replacing 'origin' with whatever you're calling the torproject master remote... git fetch origin git rebase origin/master
That way your changes don't fall too far out of date with the current HEAD (looks like you're currently 29 commits behind).
+import test.unit.util.proc
Please keep the order of the current imports (the unit/integ test imports are batched together).
- test.unit.util.proc.TestProc,
Lets move this test just above the "test.unit.util.system.TestSystem". The tests are ordered by their dependencies so that the lowest-level stuff runs first. The reason for that is that if, say, stem.util.enum breaks then it'll probably break just about everything else so we want to report those errors first (rather than leave the developer wondering why something like stem.connection was also broken).
prefix_list = sorted(list(line_prefixes))
The extra list wrapper isn't necessary.
sorted((1, 4, 2))
[1, 2, 4]
Let us know what you think about our unit tests thus far! We also wanted to let you know that we plan on finishing all proc unit tests by Tuesday (June 19), and all proc integration tests by Friday (June 22).
Great! What you have so far looks good, looking forward to seeing the rest. Be warned that, as Ravi can attest, code reviews generally take a few iterations. Here's an example...
https://trac.torproject.org/5262
Cheers! -Damian
Hi Damian,
First, to clarify our github repository situation, both Megan and I will be maintaining remote repos in github per Professor Danner's request so that we each gain experience with version control systems. As we have been working so far, I have been maintaining the mocking revisions in my mocking branch and Megan has been working on the proc testing code in her own proc branch.
Next, in continuing work on the unit tests for proc.py, we ran into another issue with the mocking code. The details are explained in the commit message, but please let me know if further clarification is necessary. The code can be found at:
https://github.com/jacthinman/Tor-Stem/blob/mocking/test/mocking.py
-Erik & Megan
On Fri, Jun 15, 2012 at 3:28 PM, Damian Johnson atagar@torproject.orgwrote:
The changes look great! I see no issues with merging to the master
branch.
Great, pushed.
Sorry about the reStructuredText inconvenience. I actually didn't even
think of it -- I just meant to clean up the comments a bit, but I'll definitely be more careful in the future.
No worries. Again, it would usually have been a welcome change. :)
Once we confirm this with Damian, he will push the changes to master in torprojects' repository.
Don't worry about that. The way that open source git projects usually work is that you make a 'pull request', which simply means saying 'I have some changes that I would like to share, here they are' (which is what you've been doing). If the changes look good then I'll rebase them onto the master branch and push them myself.
Usually only a tiny number of central developers actually have push access to the master repository.
Our code can be found at:
https://github.com/meganchang/Stem/blob/proc-tests/test/unit/util/proc.py
Ok. Previously I was pulling from 'jacthinman', is 'meganchang' the github repository where you plan to do most of your future work?
I'd suggest periodically running the following, replacing 'origin' with whatever you're calling the torproject master remote... git fetch origin git rebase origin/master
That way your changes don't fall too far out of date with the current HEAD (looks like you're currently 29 commits behind).
+import test.unit.util.proc
Please keep the order of the current imports (the unit/integ test imports are batched together).
- test.unit.util.proc.TestProc,
Lets move this test just above the "test.unit.util.system.TestSystem". The tests are ordered by their dependencies so that the lowest-level stuff runs first. The reason for that is that if, say, stem.util.enum breaks then it'll probably break just about everything else so we want to report those errors first (rather than leave the developer wondering why something like stem.connection was also broken).
prefix_list = sorted(list(line_prefixes))The extra list wrapper isn't necessary.
sorted((1, 4, 2))
[1, 2, 4]
Let us know what you think about our unit tests thus far! We also wanted
to let you know that we plan on finishing all proc unit tests by Tuesday (June 19), and all proc integration tests by Friday (June 22).
Great! What you have so far looks good, looking forward to seeing the rest. Be warned that, as Ravi can attest, code reviews generally take a few iterations. Here's an example...
https://trac.torproject.org/5262
Cheers! -Damian
Hi Eric.
First, to clarify our github repository situation...
gotcha
Next, in continuing work on the unit tests for proc.py, we ran into another issue with the mocking code.
Nice catch, though for your example (os.readlink) won't this make the tests platform dependent? Currently Beck (another volunteer working on stem) is working on making the tests run on Windows and it would be sad if we made his life harder. ;)
Minor gity side note, there is almost no use case where you should be merging 'remotes/torproject/master' into your branch. Either rebase onto 'remotes/torproject/master' or make a new branch on 'remotes/torproject/master' instead. That will make your dag (directed acyclic graph - the parent/child relationships between commits) far cleaner. Otherwise I need to cherry-pick rather than merge your work since merging would pull extra commits and confuse the master's dag. Let me know if you have any questions about this - admittedly without a whiteboard git can be a little confusing for new users.
Cheers! -Damian
The way the os module seems to reference posix, I don't believe we will run into any platform dependencies. Since os determines what environment it is in then references either itself or an appropriate external module (such as posix) in __dict__, it should always work.
With os.readlink, the issue was that inspect.getmodule(target) was returning 'posix', so posix.__dict__ was updated when os.readlink was mocked. os.__dict__, however, was not updated, so calling os.readlink() resulted in posix.readlink() rather than our mocked function.
If this doesn't seem correct, I would appreciate your feedback.
As for merging torproject/master into our branches, I'm afraid I don't know how that happened. Should I revert to before that commit and rebase, or was this a one-time issue that we simply need to avoid in the future?
Best, Erik & Megan
On Tue, Jun 19, 2012 at 3:25 PM, Damian Johnson atagar@torproject.orgwrote:
Hi Eric.
First, to clarify our github repository situation...
gotcha
Next, in continuing work on the unit tests for proc.py, we ran into
another issue with the mocking code.
Nice catch, though for your example (os.readlink) won't this make the tests platform dependent? Currently Beck (another volunteer working on stem) is working on making the tests run on Windows and it would be sad if we made his life harder. ;)
Minor gity side note, there is almost no use case where you should be merging 'remotes/torproject/master' into your branch. Either rebase onto 'remotes/torproject/master' or make a new branch on 'remotes/torproject/master' instead. That will make your dag (directed acyclic graph - the parent/child relationships between commits) far cleaner. Otherwise I need to cherry-pick rather than merge your work since merging would pull extra commits and confuse the master's dag. Let me know if you have any questions about this - admittedly without a whiteboard git can be a little confusing for new users.
Cheers! -Damian
The way the os module seems to reference posix, I don't believe we will run into any platform dependencies...
Ahhh, gotcha. My understanding of the usage was backwards (I thought that you planned to provide posix as the new argument). In that case looks good to merge, will do tomorrow morning.
As for merging torproject/master into our branches, I'm afraid I don't know how that happened. Should I revert to before that commit and rebase, or was this a one-time issue that we simply need to avoid in the future?
Not necessary, I can just cherry-pick it. Just mentioned it for the future.
What most likely happened was that you used 'git pull' rather than 'git fetch' (pull is the same as doing a fetch followed by a merge).
-
Damian Johnson -
Erik I Islo