Opened 5 years ago

Closed 5 years ago

#5472 closed defect (fixed)

Stem version parser matches git hash too

Reported by: neena Owned by: neena
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Stem's version parser considers the (git-hash) from tor --version output as a part of the version.

Ex: (git-65420e4cb5edcd02) in
Tor version 0.2.3.10-alpha-dev (git-65420e4cb5edcd02).

I have fixed this and added a test for get_system_tor_version.

I also made the regex stricter by having it reject version strings whose status_tags have a space in them and properly matching the dot between version integers (\. instead of .).

My branch with these fixes -- http://repo.or.cz/w/stem/neena.git/shortlog/refs/heads/fix-version-parsing

Right now, this breaks a few other things. Primarily because GETINFO version returns this:

250-version=0.2.3.10-alpha-dev (git-65420e4cb5edcd02)

One way to fix this would be to stop at the first space in runner.get_tor_version too. I'm going to do that unless someone suggests something better.

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by neena

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

comment:2 follow-up: Changed 5 years ago by atagar

  • Status changed from new to needs_revision

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.

comment:3 Changed 5 years ago by atagar

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.

comment:4 in reply to: ↑ 2 Changed 5 years ago by neena

  • Status changed from needs_revision to needs_review

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.

comment:5 follow-up: Changed 5 years ago by atagar

  • Status changed from needs_review to needs_revision

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

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

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

comment:6 in reply to: ↑ 5 Changed 5 years ago by neena

  • Status changed from needs_revision to needs_review

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.

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

Yeap :)

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

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

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

comment:7 follow-up: Changed 5 years ago by atagar

  • Status changed from needs_review to needs_revision

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

comment:8 in reply to: ↑ 7 Changed 5 years ago by neena

  • Status changed from needs_revision to needs_review

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?

comment:9 Changed 5 years ago by atagar

  • Resolution set to fixed
  • Status changed from needs_review to closed

comment:10 follow-up: Changed 5 years ago by atagar

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

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

Thanks! -Damian

comment:11 in reply to: ↑ 10 Changed 5 years ago by neena

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to atagar:

  1. Are you alright with this patch being a joint work with me (ie, sharing the copyright)?

Yes.

  1. Are you alright with your future stem submissions being joint works so I don't need to keep asking?

Yes.

comment:12 follow-up: Changed 5 years ago by atagar

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

comment:13 in reply to: ↑ 12 Changed 5 years ago by neena

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.