#32609: Improve practracker unit tests, and run them in "make check" and pre-commit -------------------------------------------------+------------------------- Reporter: teor | Owner: teor Type: enhancement | Status: | needs_review Priority: Medium | Milestone: Tor: | 0.4.3.x-final Component: Core Tor/Tor | Version: Severity: Normal | Resolution: Keywords: test, network-team-roadmap-november | Actual Points: 0.4 Parent ID: #32522 | Points: 0.4 Reviewer: nickm | Sponsor: | Sponsor31-can -------------------------------------------------+-------------------------
Comment (by nickm): Replying to [comment:4 teor]: > Replying to [comment:3 nickm]: > > > This isn't quite the fix I wanted, so I'd like your advice. > > > > The commits themselves look okay to me. Is the issue the one that you mention here? > > Yes, the `#include "feature/dircache/dirserv.c"` in test_controller.c. > > > > Is there a way we can restrict includes to ".h" and ".inc"? > > > Perhaps with some custom code in practracker includes.py? > > > > We could add a .may_include to all directories, starting with "*.h" and "*.inc". Then practracker would start looking at them. Does that do what you want? > > No, because practracker just logs a warning, which most people ignore. (And CI doesn't fail.) We really do need to fail on weird include patterns like this. The .may_includes checker in practracker has two entry points: the one in practracker.py, and the one in includes.py directly that replaces our old checkIncludes.py. The latter does fail on violations, if the .may_includes file is not marked as advisory. For example, add a .may_include file to src/test containing only '*.h', and run "make check". When I try this, "make check" fails. > > Right now it looks like there are some places where we include .c files intentionally. And arguably we should rename ".i" to ".inc" > > > > {{{ > > [1424]$ git grep '#include' src/ |grep -v '.\(h\|inc\)[>"]' > > src/ext/ed25519/donna/ed25519_tor.c:#include "test-internals.c" > > src/ext/ed25519/donna/fuzz/ed25519-donna-sse2.c:#include "../ed25519.c" > > src/ext/ed25519/donna/fuzz/ed25519-donna.c:#include "../ed25519.c" > > src/ext/timeouts/bench/bench-wheel.c:#include "timeout.c" > > src/ext/timeouts/lua/timeout-lua.c:#include "timeout.c" > > src/ext/timeouts/timeout.c:#include "ext/timeouts/timeout-bitops.c" > > src/lib/evloop/timers.c:#include "ext/timeouts/timeout.c" > > src/lib/fs/files.c:#include "ext/getdelim.c" > > src/lib/string/compat_string.c:#include "ext/strlcpy.c" > > src/lib/string/compat_string.c:#include "ext/strlcat.c" > > src/lib/version/git_revision.c:#include "micro-revision.i" > > src/lib/version/git_revision.c:#include "micro-revision.i" > > }}} > > If we: > * ignore "ext" (including and being included), and > * rename micro-revision.i to micro-revision.inc, > then we can restrict includes to ".h" and ".inc". I think that this general approach (ignoring ext and preferring .inc) is okay; so is listing files explicitly in .may_includes. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32609#comment:5> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs