[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #5472 [Stem]: Stem version parser matches git hash too
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs