Opened 7 months ago

Last modified 7 months ago

#28743 new defect

Tinytest library leaks out into non-test codebase

Reported by: karalabe Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Trivial Keywords:
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This isn't a bug per se, rather a peculiarity in the code organization that I've encountered, which I think is accidental. Please correct me if I'm wrong.

I've been trying to wrap the 0.3.5 series of Tor in Go (https://github.com/ipsn/go-libtor). It's working properly as far as I can tell, just making my final tweaks before pushing it. To keep my wrapper small, I've discarded some of the sources that are not relevant for building the library, namely src/tools and src/test.

To my surprise, my build failed (it worked correctly for 0.3.3), and it turned out that the tinytest library (at least half of it) is in src/ext, which refers to the src/test folder that I just deleted (the test folder refers to tinytest, circular dependency, but that's beyond the point here).

Working around it wasn't too big of a deal, I just ignored the tinytest files, but was wondering if this was an accidental leakage of test files into the library part, adding a dependency to the tests? Shouldn't the 4 tinytest files from src/ext be moved into src/test?

Child Tickets

Change History (4)

comment:1 Changed 7 months ago by teor

Cc: nickm added
Type: enhancementdefect

Thanks for using Tor, and for reporting this issue.

Tor depends on tinytest's mocking macros.
The macros don't do anything in non-test code, but they're still a compile-time dependency.
But it's weird that tinytest refers to src/test. We don't want circular dependencies like that.
nickm wrote tinytest, I'll see if he has any more insight.

Tor's in-process API is quite new, and we're still testing it.
If you find any other bugs, please let us know.

comment:2 Changed 7 months ago by nickm

Hang on though, the mocking macros aren't part of the tinytest code at all. They're in lib/testsupport.

comment:3 Changed 7 months ago by karalabe

Tinytest is part of src/ext per https://gitweb.torproject.org/tor.git/tree/src/ext?h=release-0.3.5 , which seems odd given that it's a test utility (why not put it into src/test).

The tinytest.c file contains defines TINYTEST_POSTFORK in https://gitweb.torproject.org/tor.git/tree/src/ext/tinytest.c?h=release-0.3.5#n28, which causes it to add a link dependency for void tinytest_prefork() and void tinytest_postfork() per https://gitweb.torproject.org/tor.git/tree/src/ext/tinytest.c?h=release-0.3.5#n122 .

Those methods are defined in src/test/testing_common.c per https://gitweb.torproject.org/tor.git/tree/src/test/testing_common.c?h=release-0.3.5#n230 .

My annoyance is really with code organization, that a file in a library requires stuff defined in a test harness.

comment:4 Changed 7 months ago by teor

Milestone: Tor: unspecified
Note: See TracTickets for help on using tickets.