Opened 4 years ago

Closed 4 years ago

#8949 closed enhancement (fixed)

Add a way to do mocking for Tor unit tests

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client testing
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now, one of our biggest obstacles to improved test coverage is the fact that we don't have a good way to mock functions during C testing.

There are a big pile of mocking tools for C, but they mostly suck and either want you to split your code up weirdly, litter it with macro salad, or such.

There's another class of solution based on preprocessing C or postprocessing assembly, but both approaches are fragile, and we'd probably need to polish or maintain whatever tool we wound up using. Not a great situation to be in. I've maintained compiler wrappers. (It was a bad scene, man.)http://www.pmg.csail.mit.edu/polyj/

Still, our current low test coverage is worse still; worse even than macro salad; worse even than magic compiler wrappers. So we ought to bite the bullet and figure out the least difficult and least awful option and do it.

(Please don't just post the results of a search on "c mocking tools" or "c unit testing" here: I can do that search too.)

Child Tickets

TicketTypeStatusOwnerSummary
#9261defectclosedAvoid 'unused parameter' warning
#9263defectclosedTraditional /bin/sh is unhappy about some file globbing

Change History (10)

comment:1 Changed 4 years ago by nickm

Status: newneeds_review
Summary: Add a robust way to do mocking for Tor unit testsAdd a way to do mocking for Tor unit tests

Please review branch "fancy_test_tricks". It's got a very small amount of mocking support, and a great deal of infrastructure to make the mocking support usable.

The mocking methodology here is "the simplest thing that could work" -- it's one of the ones that festoon the code with macro salad, and uglifies the declarations of functions that are going to get mocked. It has the advantage of being portable, robust, and comprehensible.

comment:2 Changed 4 years ago by andrea

Begin code review:

f7d654b81e1a65803c22bb53fc1d3a2021d87d50:

  • By modifying src_or_libtor_testing_a_CPPFLAGS, I presume the intent is to build everything twice, once for the tor binary and once for the unit tests.
  • I am deficient in automake clue here; are we certain this builds a complete separate set of object files, rather than just seeing one built and then not trying to rebuild it?
  • Building everything twice seems icky, but I agree the other options seem even ickier.

comment:3 Changed 4 years ago by andrea

a3e0a87d951b1323ca542c9b115d5525d0d022c9 looks fine to me

comment:4 Changed 4 years ago by andrea

17e9fc09c31dffdc647385220a14daef5a35273a looks okay

comment:5 Changed 4 years ago by andrea

4753ad4f1dbd7fa3233ead5770e9c8bd619b8d07 looks good too

comment:6 Changed 4 years ago by andrea

b6e8c74667cf723c5ef4d081fc901752e05f9a9b looks okay

comment:7 Changed 4 years ago by andrea

ec6c155f827000e337796f1f1c54299fbc5cf72a looks good

comment:8 in reply to:  2 Changed 4 years ago by nickm

Replying to andrea:

Begin code review:

f7d654b81e1a65803c22bb53fc1d3a2021d87d50:

  • By modifying src_or_libtor_testing_a_CPPFLAGS, I presume the intent is to build everything twice, once for the tor binary and once for the unit tests.

That's right. When you specify the same C file as a source for two different build targets (like libor.a and libor-testing.a), and say that those build targets have different CPPFLAGS or CFLAGS or whatever, then autoconf will build the C file twice: first storing it in foo.o, and then in (say) src_or_libor_testing_a_foo.o or something like that.

  • I am deficient in automake clue here; are we certain this builds a complete separate set of object files, rather than just seeing one built and then not trying to rebuild it?

It does in fact literally cause there to be two object files made for each C file. :)

  • Building everything twice seems icky, but I agree the other options seem even ickier.

Agreed.

Linus is having a look too; once he's done, or once a little while passes, I plan to merge.

comment:9 Changed 4 years ago by ln5

I've been skimming the changes. Nothing jumped at me.

I've been running make check with and without --enable-coverage on a
FreeBSD system with gcc 4.2.1. Works as expected as far as I can
tell.

comment:10 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks for the reviews; merging!

Note: See TracTickets for help on using tickets.