[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