2

It's generally accepted that using eval is bad practice. The accepted answer to this question states that there is almost always a better alternative. However, the timeit module in the standard library uses it, and I stumbled onto a case where I can't find a better alternative.

The unittest module has assertion functions of the form

self.assert*(..., msg=None)

allowing to assert something, optionally printing msg if it failed. This allows running code like

for i in range(1, 20):
    self.assertEqual(foo(i), i, str(i) + ' failed')

Now consider the case where foo can raise an exception, e.g.,

def foo(i):
    if i % 5 == 0:
        raise ValueError()
    return i

then

  1. On the one hand, msg won't be printed, as assertEqual was technically never called for the offending iteration.
  2. On the other hand, fundamentally, foo(i) == i failed to be true (admittedly because foo(i) never finished executing), and so this is a case where it would be useful to print msg.

I would like a version that prints out msg even if the failure cause was an exception - this will allow to understand exactly which invocation failed. Using eval, I could do this by writing a version taking strings, such as the following (which is a somewhat simplified version just to illustrate the point):

def assertEqual(lhs, rhs, msg=None):
    try:
        lhs_val = eval(lhs)
        rhs_val = eval(rhs)
        if lhs_val != rhs_val:
            raise ValueError()
    except:
        if msg is not None:
            print msg
        raise

and then using

for i in range(1, 20):
    self.assertEqual('foo(i)', 'i', str(i) + ' failed')

Of course technically it's possible to do it completely differently, by placing each call to assert* within a try/except/finally, but I could only think of extremely verbose alternatives (that also required duplicating msg.)

Is the use of eval legitimate here, then, or is there a better alternative?

Community
  • 1
  • 1
Ami Tavory
  • 74,578
  • 11
  • 141
  • 185

1 Answers1

4

If an exception is raised unexpectedly, that would point to a bug in your code. Exactly the case you want to discover with your unit tests. It's not simply not equal, it's a bug you discovered that you need to fix.

If you expect an exception to be raised, assert that with:

with self.assertRaises(ValueError):
    foo(i)

If you expect no exception to be raised, use:

try:
    foo(i)
except ValueError:
    self.fail("foo() raised ValueEror unexpectedly!")

If anything, I'd suggest you write your own wrapper like:

self.assertEqualsAndCatch(foo, i, msg=...)

I.e.: pass a callback and its arguments, instead of a string to eval.

deceze
  • 510,633
  • 85
  • 743
  • 889
  • Thanks for your answer. I believe that what you suggest here is exactly what I wrote in the penultimate paragraph in my question as being a viable, yet verbose, alternative. I'd prefer avoiding having each call enclosed by a `try/except/finally` construct - it lowers the code's SNR. – Ami Tavory Apr 01 '16 at 13:18
  • I don't see it as verbose. Granted, it takes *extra lines*. But it's readable, it provides the relevant information when a failure happens that should not. Because, as stated by deceze's answer, if your unittest actually raises something, that means you caught a bug you should have tested for. – idjaw Apr 01 '16 at 13:20
  • @Ami I don't think you should be *asserting* anything when you *expect* an exception to be raised! Looping over 20 integers knowing that half of them *won't* work, yet writing an assert that *expects* them to work is weird. If you take out the positive assertion, there's no duplicate message and it's not terribly verbose either. – deceze Apr 01 '16 at 13:21
  • @deceze Thanks again for your comment. I am not *expecting* anything to be raised - If I were, I'd use `assertRaises`. – Ami Tavory Apr 01 '16 at 13:23
  • 1
    @Ami That's my point then: if you expect that *no* exception be raised and to just receive a comparable result, then your unit test should do a plain `assertEquals`; if and when an exception actually *is* raised: congratulations, you found a bug that must be fixed! You should absolutely not suppress that into a simple "oops, not equal". – deceze Apr 01 '16 at 13:25
  • I agree. Especially as a developer, you do not want to suppress a Stacktrace. You *want* to know what happened. Because it should not have happened. – idjaw Apr 01 '16 at 13:26
  • @idjaw This answer is good and has its points. However, I'd have to disagree on verbosity, though - imagine if this were repeated in may `unittest` methods - this construct would repeat itself. To me, that's an indication that there's boilerplate code and verbosity going on. Having said that, again, it's not that I think this answer is not good. I am hoping for an alternative approach, though (using `eval` or preferably not). – Ami Tavory Apr 01 '16 at 13:27
  • @idjaw I do not think that the stacktrace must be suppressed, though - this is a minimal version of the code, but there are facilities for printing the stacktrace or sending the exception higher. – Ami Tavory Apr 01 '16 at 13:28
  • 1
    @Ami What I'm trying to tell you is that I think your premise is entirely flawed. Why would you want to handle an ***exception***, an *exceptional case* which should not happen, the same as a faulty calculation? Perhaps your API is badly designed and is raising exceptions as a "normal" case...? – deceze Apr 01 '16 at 13:29
  • @deceze Would you agree, though, that if `assertEqual` is called within a loop (which is not uncommon) then the version in the question (problematic as it is) prints out the iteration without adding more LOCs? OH, BTW - regarding your question, then no - I'm not referring to a case where exceptions are raised as some sort of "return value". – Ami Tavory Apr 01 '16 at 13:30
  • 1
    @Ami 1) If you call it in a loop, all values in the loop should yield the same result: either success or an expected failure. The expected result shouldn't change halfway through the loop. Remember: you're always writing tests for the *expected* outcome; you tell unittest what you *expect*, and it alerts you to cases where that expectation fails. 2) If you write it in a loop, there isn't a lot of LOC overhead; you can cover 20 calls to your function with the same `try..catch` statement; so I don't see the big problem. – deceze Apr 01 '16 at 13:34
  • 1
    @Ami If anything, I'd suggest you write your own wrapper like `self.assertEqualsAndCatch(foo, i, msg=...)`. I.e.: pass a callback and its arguments, instead of a string to `eval`. – deceze Apr 01 '16 at 13:35
  • 1
    @deceze OK, many thanks, esp. for the last comment - that makes a lot of sense to me. – Ami Tavory Apr 01 '16 at 13:35
  • 1
    @deceze If you'd care to write your last comment in the answer, I'd be happy to accept it (not that you really need the rep, but still - I think it's a valuable answer.) – Ami Tavory Apr 01 '16 at 13:37
  • 1
    @Ami Agreed, that's a useful addition. – deceze Apr 01 '16 at 13:39
  • 2
    Just thought I'd say this was a great thread to follow. :) Wish there were more of these good conversations. – idjaw Apr 01 '16 at 13:40
  • 1
    @ldjaw Thanks for your contribution as well, as well as snakecharmerb. – Ami Tavory Apr 01 '16 at 13:44