Opened 5 years ago

Closed 5 years ago

#11507 closed enhancement (implemented)

New tests for status.c

Reported by: _x3j11 Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In order to increase test coverage for Tor, it would be advantageous to have some mechanism to decrease the friction involved in the mocking process a little bit. A number of macros are introduced for this purpose, and a new test suite is added that uses them to test status.c (as a bit of a proof-of-concept) that gives 100% coverage.

The idea is to think of each test being executed in its own sub-namespace. Normally, we would create these sub-namespaces by appending the module and test names to our mock functions; the attached patch introduces macros to make this simpler. These macros will also declare variables in the current test namespace that can be used to track (manually) how many times a given mock was called. Finally, macros are introduced to make the declaration of the struct testcase_t array simpler.

Some further refinements could be made (eg., having a nicer way to set up and tear down a test fixture), but this should prove a good starting point. The disadvantage of executing tests like this in their own namespace is some mocks will need to be duplicated if they are shared amongst different test cases, but it is more ideal for tests to not have dependencies on other test setups so that failures can be easily isolated.

Child Tickets

Attachments (1)

0001-Uplift-status.c-unit-test-coverage-with-new-test-cas.patch (47.7 KB) - added by _x3j11 5 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 5 years ago by _x3j11

Status: newneeds_review

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-final

Interesting! Yes, I think this token-concatenating stuff is probably a good approach.

Initial comments:

  • For reserved-identifier reasons, I'd like to not be declaring any macros or identifiers that start with an underscore. Instead, I've been using ends-with-an-underscore to indicate "don't use this".
  • Most of these newly declared mock functions and test functions should be static.
  • I think we should divide testsupport.h into parts that are used for the whole codebase (like MOCK_IMPL / MOCK_DECL) and parts that are only used in the tests themselves.
  • I like "test_main" as a generic test name slightly more than "main".
  • The TEST_CASE and TEST_CASE_ASPECT macros should expose ways to set the 'flags' fields -- or maybe they should all have the TT_FORK flag, to prevent stray MOCKs from affecting other tests.

Also, this patch doesn't pass compilation with our full warning suite enabled. (configure with --enable-gcc-warnings for all the ones your compiler supports.) Examples:

src/test/test_status.c:36:16: warning: unused parameter 'arg'
      [-Wunused-parameter]
NS(main)(void *arg)

src/test/test_status.c:87:12: warning: assigning to 'char *' from
      'const char [11]' discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
  expected = "0:00 hours";

src/test/test_status.c:317:1: warning: no previous prototype for function
      'status_log_heartbeat__not_in_consensus_main' [-Wmissing-prototypes]
NS(main)(void *arg)

(That's a first pass review -- I haven't read the tests themselves in detail yet)

comment:3 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

comment:4 Changed 5 years ago by _x3j11

Status: needs_revisionneeds_review

I've added a new patch file adopting all your suggestions.

(Elsewhere there are some other warnings being treated as errors on my system, but I think I have addressed all those that were raised by my patch. I'll try and fix these unrelated warnings separately.)

Last edited 5 years ago by _x3j11 (previous) (diff)

comment:5 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks better, thanks! Merging.

Note: See TracTickets for help on using tickets.