[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #32609 [Core Tor/Tor]: Improve practracker unit tests, and run them in "make check" and pre-commit



#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@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs