Stem version parser matches git hash too

added component::archived/stem owner::neena priority::medium resolution::fixed status::closed type::defect labels

Alternatively, we could use another variable in Version to store anything after the first space. This will usually be (git-$HASH).

Hi neena, thanks for the patch! Looks great, only a few small changes that I'd suggest.

Treat versions with spaces in them as invalid

At present the regex won't match spaces but it will match other whitespace. Maybe replace "(-[^ ])" with "(-\S)"?

Fix Version to match . strictly

Nice catch. Would you mind adding a unit test for this (maybe something like "1a2a3a4")?

Returns a function that passes calls on to the stem.util.system.call() function if it's argument doesn't begin one of the keys in outputdict

This is almost perfect, my only suggestion here would be to change the behavior when the system call isn't in outputdict. This is a unit test so, if we're making system calls from it, we're already doing something wrong (since we're making the test os dependent). Instead of passing through to system.call lets fail the test instead. This could look like...

VERSION_CALL_OUTPUT = """\
Mar 22 23:09:37.088 [notice] Tor v0.2.2.35 (git-73ff13ab3cc9570d). \
This is experimental software. Do not rely on it for \
strong anonymity. (Running on Linux i686)
Tor version 0.2.2.35 (git-73ff13ab3cc9570d)."""

class system_call_mocker:
  def __init__(self, current_test, responses):
    self.current_test = current_test
    self.responses = responses
  
  def call(self, command):
    if command in responses:
      return responses[command].splitlines()
    else:
      self.current_test.fail()

def test_get_system_tor_version(self):
  system_mocker = system_call_mocker(self, {"tor --version": VERSION_CALL_OUTPUT})
  mocking.mock(stem.util.system.call, system_mocker.call)
  version = stem.version.get_system_tor_version()
  self.assert_versions_match(version, 0, 2, 2, 35, None)

(Side Note: I didn't know about '' for multi-line string blocks. Thanks, that is gonna make a few other places much more readable...)

When you're done please put the ticket into "Needs code/patch review" so it's clear at a glance what this patch's state is.

Trac:
Status: new to needs_revision

Alternatively, we could use another variable in Version to store anything after the first space. This will usually be (git-$HASH).

Oops, forgot to comment on this. That is tempting since the git commit id could be interesting information to the caller. However, it isn't in the spec and I'd rather only include information that we can rely on being there. This information might easily disappear or change in upcoming tor versions.

Might be handy to add in the future, but for now lets leave it out.

Replying to atagar:

At present the regex won't match spaces but it will match other whitespace. Maybe replace "(-[^ ])" with "(-\S)"? Done.

Nice catch. Would you mind adding a unit test for this (maybe something like "1a2a3a4")? Done.

Returns a function that passes calls on to the stem.util.system.call() function if it's argument doesn't begin one of the keys in outputdict

This is almost perfect, my only suggestion here would be to change the behavior when the system call isn't in outputdict. This is a unit test so, if we're making system calls from it, we're already doing something wrong (since we're making the test os dependent). Instead of passing through to system.call lets fail the test instead. This could look like...

Completely missed this, fixed.

{{{ VERSION_CALL_OUTPUT = """
Mar 22 23:09:37.088 [notice] Tor v0.2.2.35 (git-73ff13ab3cc9570d).
This is experimental software. Do not rely on it for
strong anonymity. (Running on Linux i686) Tor version 0.2.2.35 (git-73ff13ab3cc9570d)."""

class system_call_mocker: def init(self, current_test, responses): self.current_test = current_test self.responses = responses

def call(self, command): if command in responses: return responses[command].splitlines() else: self.current_test.fail()

I've raised an Exception instead of using current_test.fail. 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.

I modified branch history. I'm not sure if this is the best way to make changes. pull -f should work.

Trac:
Status: needs_revision to needs_review

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...

for system.call to return that response
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. :)

Trac:
Status: needs_review to needs_revision

Replying to atagar:

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... Sorry, put it off for later and forgot. Won't happen again.

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

for system.call to return that response

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). Yeap :)

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
}}}

We won't be able to reuse the function for different outputs if we do this.

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'.

The mocking function in system is (kinda) the opposite of this. Paraphrased from the docstring, it passes calls on to stem.util.system.call if it matches a prefix, acts as a no-op otherwise. We'll have to have both anyway.

c. Using a lambda to do this. It triggers the OSError in get_system_tor_version with ("'%s' didn't have any output" % version_cmd) on failure. If that happens, it could effectively be thought of as mocking the OSError that would've been raised if this had not been a unit test? :p

{{{ def test_get_system_tor_version(self): def _call_mocker(resp): return lambda cmd: resp.splitlines() if cmd == "tor --version" else False

mocking.mock(stem.util.system.call, _call_mocker(
"""Mar 22 23:09:37.088 [notice] Tor v0.2.2.35 (git-73ff13ab3cc9570d). \

This is experimental software. Do not rely on it for
strong anonymity. (Running on Linux i686) Tor version 0.2.2.35 (git-73ff13ab3cc9570d).""")) version = stem.version.get_system_tor_version() self.assert_versions_match(version, 0, 2, 2, 35, None)


(response becomes resp because of the 80-character rule. Too neat to split)

> Sorry about having so much back-and-forth. :)
Well, I'm feeling really good about my code now. :)

**Trac**:  
**Status**: needs_revision **to** needs_review

Sorry, put it off for later and forgot. Won't happen again.

No worries, I make pydoc mistakes constantly. :)

We won't be able to reuse the function for different outputs if we do this.

True, though we don't need alternative outputs yet and if we did it will be easy to refactor. I would also argue that if/when we want multiple system_tor_version tests we should add a generator function to avoid copying the "This is experimental software." boilerplate, but that too isn't needed until we have more things to test.

The plain mock function is simpler, and keeping code simple is a very good thing. You're right that if we add several more tests later then your lambda function will be preferable. But lets not optimize for that until it happens.

Well, I'm feeling really good about my code now. :)

Great, glad that you generally like the changes. :)

Trac:
Status: needs_review to needs_revision

Replying to atagar:

The plain mock function is simpler, and keeping code simple is a very good thing. You're right that if we add several more tests later then your lambda function will be preferable. But lets not optimize for that until it happens.

Alright. In that case, I'm going to let TOR_VERSION_OUTPUT be a global var, since we can change that later too. It looks neater than returning a multiline triple quoted string anyway.

I've pushed the latest to my branch. Is there anything else that needs changing?

Trac:
Status: needs_revision to needs_review

Thanks! Looks great, merged... https://gitweb.torproject.org/stem.git/commitdiff/c63124069422cbba45f4b08b223f2bcb6eb99ef6?hp=222d58a9bad7e1675f70002fb00b2a9b962cfe63

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Hi Ravi. After replying to your proposal on tor-dev@ Sebastian pointed out that stem's license will pose a problem for code sharing with other tor projects. I've added a note explaining it to... https://trac.torproject.org/projects/tor/wiki/doc/stem#CopyrightforPatches

I'm checking with Wendy to see if this is the right interpretation, but working from that assumption a couple questions...

a. Are you alright with this patch being a joint work with me (ie, sharing the copyright)? b. Are you alright with your future stem submissions being joint works so I don't need to keep asking?

Thanks! -Damian

Trac:
Resolution: fixed to N/A
Status: closed to reopened

Replying to atagar:

a. Are you alright with this patch being a joint work with me (ie, sharing the copyright)? Yes. b. Are you alright with your future stem submissions being joint works so I don't need to keep asking? Yes.

Trac:
Status: reopened to closed
Resolution: N/A to fixed

Hi Ravi. Sorry about reopening again. I checked with Wendy (a law professor and director with the tor project) and she suggested asking for patches to be within the public domain instead. Is that alright with you?

Trac:
Status: closed to reopened
Resolution: fixed to N/A

Replying to atagar:

Hi Ravi. Sorry about reopening again. I checked with Wendy (a law professor and director with the tor project) and she suggested asking for patches to be within the public domain instead. Is that alright with you? Yes.

Trac:
Status: reopened to closed
Resolution: N/A to fixed

closed

mentioned in issue #5750 (closed)

mentioned in issue #19890 (moved)

Stem version parser matches git hash too

Child items 0

Activity