Opened 2 years ago

Closed 21 months ago

#8949 closed enhancement (fixed)

Add a way to do mocking for Tor unit tests

Reported by: nickm Owned by:
Priority: major Milestone: Tor: 0.2.5.x-final
Component: Tor Version:
Keywords: tor-client testing Cc:
Actual Points: Parent ID:
Points:

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 22 months ago by nickm

  • Status changed from new to needs_review
  • Summary changed from Add a robust way to do mocking for Tor unit tests to Add 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 follow-up: Changed 21 months 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:8 in reply to: ↑ 2 Changed 21 months 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 21 months 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 21 months ago by nickm

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

Thanks for the reviews; merging!

Note: See TracTickets for help on using tickets.