Opened 4 weeks ago

Last modified 4 weeks ago

#26420 new enhancement

Testing - specify literal patterns instead of regex patterns

Reported by: dmr Owned by: dmr
Priority: Very Low Milestone:
Component: Core Tor/Stem Version:
Severity: Minor Keywords: dev testing code-improvement
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: atagar Sponsor:

Description (last modified by dmr)

w.r.t. 7711050619af1a2f8ecf4aa87f774baa5f367b3b, I was planning to file this ticket anyways, so might as well now for the discussion.

atagar linked this StackOverflow answer in the commit message.

(I was a bit behind on filing this ticket, but already started doing the literal re.escape() bit in my test cases. Hence atagar's comment in the commit.)

Anyway, here's the ticket text I had started to prep - now slightly tweaked:

The testing codebase makes pretty extensive use of unittest.TestCase.assertRaisesRegexp().
An example is here:

  def test_no_common_link_protocol(self):
    """
    Connection without a commonly accepted link protocol version.
    """

    for link_protocol in (1, 2, 6, 20):
      self.assertRaisesRegexp(stem.SocketError, 'Unable to establish a common link protocol with 127.0.0.1:1113', Relay.connect, '127.0.0.1', test.runner.ORPORT, [link_protocol])

The second argument is treated as a regex pattern, so it will actually match more than intended - possibly leading to some false negatives (although unlikely in this example).

The use of unittest.TestCase.assertRaisesRegexp() without re.escape() for a literal expression is decently common - the use of it intended for a regex is fairly rare (I haven't come across a test yet that I can recall).

Having a "literal" check is possible in (at least) two ways:

with self.assertRaises(SomeException) as cm:
  do_something(some_param)
self.assertEqual(str(cm.exception), expected_literal)
self.assertRaisesRegexp(SomeException, '^%s$' % re.escape(expected_literal), do_something, some_param) 

Importantly, both of these check for exact string.
Much of the codebase doesn't use re.escape(), and where it does, I didn't see any ^ or $ (although I didn't search exhaustively), so that could match on substrings, also allowing for subtle bugs.

So we may consider a helper class StemTestCase(unittest.TestCase) which adds an assertRaisesLiteral() method, to make it easier to do this. (Or some other means of adding that in.)

We could of course take the second option with re.escape(), but since a lot of the codebase already doesn't seem to do that, it's probably quite easy to forget, especially the '^%s$' % part.

atagar: thoughts on these options? or leave things as-is / wontfix?

(Filing this as a task ticket, as it's probably a discussion point more than anything else. I'd expect from the edge cases, there could be some defects, some enhancements.)

See especially atagar's comment about implementation thoughts.

Child Tickets

Change History (5)

comment:1 Changed 4 weeks ago by dmr

Status: assignedneeds_review

Switching to needs_review. I feel that makes the most sense to bounce the discussion forward to you, atagar! :)

comment:2 Changed 4 weeks ago by atagar

Hi Dave. I hesitate to mention this but option would be to add an assertRaisesWith() method. We already wrap unittest's TextTestRunner to record test runtimes and add python 2.6 support for a couple methods...

https://gitweb.torproject.org/stem.git/tree/stem/util/test_tools.py#n270

Adding this method would be simple. The only thing that makes me hesitate is it's definitely non-standard.

If you'd care to migrate Stem to a simple assertRaisesWith() method then feel free. Or not. Happy to go either way. :)

comment:3 in reply to:  2 Changed 4 weeks ago by dmr

Priority: MediumVery Low
Severity: NormalMinor

Replying to atagar:

Hi Dave. I hesitate to mention this but option would be to add an assertRaisesWith() method. We already wrap unittest's TextTestRunner to record test runtimes and add python 2.6 support for a couple methods...

https://gitweb.torproject.org/stem.git/tree/stem/util/test_tools.py#n270

Thanks for the pointer! I was not aware of this wrapping in the code.

Adding a versioned link here for posterity:
https://gitweb.torproject.org/stem.git/tree/stem/util/test_tools.py?id=0192b29a4784465e5f69f11ced584a54644e4a90#n270

Adding this method would be simple. The only thing that makes me hesitate is it's definitely non-standard.

I agree. The nonstandard aspect was why I was thinking to subclass it. That would be at least mildly more obvious.

If you'd care to migrate Stem to a simple assertRaisesWith() method then feel free. Or not. Happy to go either way. :)

I think it's not a high priority (changing to Very Low), as I think it's unlikely that we're getting any false negatives.

comment:4 Changed 4 weeks ago by dmr

Keywords: code-improvement added

Tagging with the new code-improvement keyword. (Added to the wiki, too)

comment:5 Changed 4 weeks ago by dmr

Description: modified (diff)
Status: needs_reviewnew
Summary: Discuss: Testing - specify literal patterns instead of regex patternsTesting - specify literal patterns instead of regex patterns
Type: taskenhancement

Since this is no longer really a discussion, swapping some aspects of the ticket.

Note: See TracTickets for help on using tickets.