Opened 5 years ago

Closed 5 years ago

#12955 closed enhancement (implemented)

New tests for routerset.c

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

Description

New tests are attached for routerset.c that are in the same style as introduced in #11507. This provides a 100% coverage test suite for this module.

Child Tickets

Attachments (1)

0001-Introduce-full-coverage-tests-for-module-routerset.c.patch (63.9 KB) - added by _x3j11 5 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 5 years ago by arma

Status: newneeds_review

comment:2 Changed 5 years ago by nickm

Keywords: tor-client added
Type: defectenhancement

Wow, thorough. Awesome.

Two quick questions, though.

First, why add the -g to the cflags in src/test/include.am ? It should be getting added to CFLAGS by configure.ac, right?

Second, why the 'return NULL' mocking for strmap_new() and smartlist_new() and digestmap_new() ? They let us make sure that the components of the routerset really get initialized, but I think this approach creates a few problems:

  • It possibly over-fits the specific implementation for routerset: there's nothing written stone saying that a routerset must allocate N smartlists, M digestmaps, etc.
  • It makes the functions behave differently from their real versions. (The real strmap_new() can never return NULL, for example.)

I wouldn't mind these issues so much if the tests were clearly marked to say which ones were glass-box tests that are expected to break if the implementation of routerset changes, and which are black-box tests that represent real requirements on the behavior of routerset that ought to be met by any implementation of routerset.

What do you think?

comment:3 Changed 5 years ago by _x3j11

First, why add the -g to the cflags in src/test/include.am ? It should be getting added to CFLAGS by configure.ac, right?

Oh, that -g in cflags might have been a leftover thing when I was debugging. Let me doublecheck if -g gets added properly, otherwise, I'll remove it.

It possibly over-fits the specific implementation for routerset: there's nothing written stone saying that a routerset must allocate N smartlists, M digestmaps, etc.

It's hard to get the right balance between over-fitting as you describe and thoroughness. The routerset_new test is probably guilty of that, true. Perhaps it's just sufficient to test the routerset's members are non-NULL and check that the *_new methods have been called. (The *_new methods just return NULL out of "laziness" -- I was trying to avoid creating the real objects here, but on further musing, they could just as easily return empty blobs of memory. Let me try and rejig that)

I'll have a scan through and see if there's any other serious instances of overfitting like this, and try and weaken them a little.

It makes the functions behave differently from their real versions. (The real strmap_new() can never return NULL, for example.)

This is tricky and I'm not sure what is appropriate here: should test-writing require cognizance of the behavior of modules not under test? how much codification of specific inter-module contracts be done in tests? given that there are asserts throughout e.g., on malloc failing, do we codify the absence of these with tests? can we assert (positive/negative) that an assert (abort) has tripped with the current test infrastructure? (I don't think that's true at the moment, but it's something we should do. I'll look into that.)

I wouldn't mind these issues so much if the tests were clearly marked to say which ones were glass-box tests that are expected to break if the implementation of routerset changes, and which are black-box tests that represent real requirements on the behavior of routerset that ought to be met by any implementation of routerset.

That seems reasonable. I'll attempt to do that as well.

comment:4 Changed 5 years ago by _x3j11

I've added a new version of the patch. This version

  • adds comments to all tests, specifically noting whether a test is functional (blackbox), or structual (whitebox).
  • does not add the -g flag from include.am
  • weakens the tests for routerset_new and routerset_free, specifically, only testing whether the members of the routerset are non-null and only tests that the *_free functions are called a nonzero amount of times. (I think those were the only real instances of overfitting in the test suite, but let me know if something else looks objectionable.)

Let me know if there's anything else you think that needs improvement.

comment:5 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Seems better now; thanks! Applied to master.

Note: See TracTickets for help on using tickets.