[tor-bugs] #17076 [Tor]: Improve coverage on src/or/config.c (options_validate)

[Tor Bug Tracker & Wiki]

#17076: Improve coverage on src/or/config.c (options_validate)
----------------------------------+------------------------------------
 Reporter:  rjunior               |          Owner:
     Type:  enhancement           |         Status:  reopened
 Priority:  Medium                |      Milestone:  Tor: 0.2.8.x-final
Component:  Tor                   |        Version:
 Severity:  Normal                |     Resolution:
 Keywords:  testing, 028-triaged  |  Actual Points:
Parent ID:                        |         Points:  small
  Sponsor:  SponsorS              |
----------------------------------+------------------------------------

Comment (by teor):

 Replying to [comment:35 olabini]:
 > When it comes to this:
 >    never rely on the developer to choose a set of default options that
 validate correctly
 >       instead, supply a set of default options that validate correctly
 at the top of the file, and use them throughout the file, overriding
 individual options as needed
 >
 > I'm not sure I understand exactly what you mean. The problem is that the
 current setup is tuned to reach as many branches as possible.

 Oops, sorry, the branch I didn't mention at all was
 feature17840-v11-merged in #17840 / #18132.

 Tuning the unit tests to reach as many branches as possible is not our
 highest priority. Writing robust unit tests that test the important
 invariants in the implementation (and only important invariants) is more
 important.

 Covering all the branches makes it really hard to change anything, or
 refactor anything, because everything has to behave exactly like it did
 before, even down to the level of unimportant behaviour like the order of
 the checks in options_validate().

 While the fact that the check is performed is important, the fact that it
 happens before another check is rarely significant. Sometimes it could be
 significant if the check is in a tightly-coupled group of checks on the
 same or similar options.

 That said, the current setup could be modified to reduce the number of
 warning messages issued. We normally prefer our unit tests to complete
 without warnings, in this case, they should complete with the least number
 of warnings necessary to perform the test. Some of the unit tests had many
 unnecessary warnings because appropriate default options weren't set. This
 shouldn't affect coverage (much), as long as those options are tested
 elsewhere.

 > The real solution here is actually to separate out options_validate into
 smaller functions that can be tested in isolation, instead of testing the
 full options_validate - the goal of testing all branches in that function
 was as a step to make it possible to refactor the function into something
 more manageable.

 I agree, it's incredibly long and needs to be refactored. But we don't
 need 100% coverage to do that, we need coverage of all the important
 invariants in the code.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17076#comment:36>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online