[tor-bugs] #5472 [Stem]: Stem version parser matches git hash too

[Tor Bug Tracker & Wiki]

#5472: Stem version parser matches git hash too
--------------------+-------------------------------------------------------
 Reporter:  neena   |          Owner:  neena         
     Type:  defect  |         Status:  needs_revision
 Priority:  normal  |      Milestone:                
Component:  Stem    |        Version:                
 Keywords:          |         Parent:                
   Points:          |   Actualpoints:                
--------------------+-------------------------------------------------------
Changes (by atagar):

  * status:  needs_review => needs_revision


Comment:

 > I also did not seperate VERSION_CALL_OUTPUT because it will be
 straightforward to add more cases to test_get_system_tor_version later
 without putting too much variables in the file's global namespace.

 Makes sense. Personally I still favor moving it to a global in order to
 keep the function's indentation (imho it makes the file easier to quickly
 scan over), but as the author if this patch I'll leave it to your
 discretion.

 > I modified branch history. I'm not sure if this is the best way to make
 changes.

 It's discouraged in the git world if you have anonymous people cloning
 your repository (since it causes issues for the puller) but since it's
 just us this is fine. Actually, it's preferable for me since it makes for
 a nicer history when I merge. Thanks for the warning though.

 > responses is a dictionary of prefix:output pairs. prefix should be a
 string and
 > output should either be a string-like object or a list of strings.

 Minor thing but please replace this comment with the method for
 documenting arguments used throughout the codebase...

 {{{
 Arguments:
   responses (dict) - prefix/output pair where the output can be a string
 or list
                      of lines

 Returns:
   str with the mocked output of the system call
 }}}

 On reflection this is probably a little more than we need. For this test
 we just want two things...

 1. for system.call to return that response
 2. for 'something failure-ish' to happen if system.call is not called with
 'tor --version'

 We don't need prefix matching, str/list responses, or anything else that's
 fancy (I'm assuming that you copied this from the system integ tests where
 these features were needed). I'd favor either of a couple options...

 a. Simplify this mocking to be specific to the test, this would make the
 test...

 {{{
 def test_get_system_tor_version(self):
   def _mock_call(command):
     if command == "tor --version":
       return <stuff>
     else:
       raise ValueError("system.call received an unexpected command: %s" %
 command)

   mocking.mock(stem.util.system.call, _mock_call)
   ... rest of the test
 }}}

 (Minor note: It's generally a good idea not to throw the general Exception
 class since it does not give any information about what went wrong. In
 this case it's the value of an input argument, so a ValueError would be
 appropriate.)

 b. Alternatively we could reuse the system test's call mocking function by
 moving it to 'test.mocking'. However, I favor option 'a' until we have a
 test that needs to do that style of mocking on 'system.call'.

 Sorry about having so much back-and-forth. :)

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5472#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online