Opened 2 months ago

Closed 8 weeks ago

#32609 closed enhancement (implemented)

Improve practracker unit tests, and run them in "make check" and pre-commit

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: test, network-team-roadmap-november
Cc: nickm Actual Points: 0.4
Parent ID: #32522 Points: 0.4
Reviewer: nickm Sponsor: Sponsor31-can

Description


Child Tickets

Change History (7)

comment:1 Changed 2 months ago by teor

Reviewer: nickm
Status: assignedneeds_review
Summary: Improve practracker unit tests, and run them in "make check"Improve practracker unit tests, and run them in "make check" and pre-commit

comment:2 Changed 2 months ago by teor

This isn't quite the fix I wanted, so I'd like your advice.

I noticed that we merged a commit that included an entire ".c" file in a unit test, and I'd like "make check" to fail when that happens.

Is there a way we can restrict includes to ".h" and ".inc"?
Perhaps with some custom code in practracker includes.py?

(I fixed the issue in d7fdab49f79c6c1ba3e0c8147ad9242f09d29f2c, but I'm concerned it got past review.)

comment:3 Changed 2 months ago by 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?

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?

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"

comment:4 in reply to:  3 ; Changed 2 months ago by teor

Replying to 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.

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".

comment:5 in reply to:  4 Changed 8 weeks ago by nickm

Replying to teor:

Replying to 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.

comment:6 Changed 8 weeks ago by teor

See my PR:

It will need to be squashed before merging.

I went with the simple change, and added the missing .may_include files. There are 4 !advisory files right now, in core/{crypto,mainloop,or,proto}. They won't catch "*.c" includes, but the others will. That's probably ok for now.

We can get more specific about the dependencies in future work.

I documented !advisory in practracker/includes.py, because it wasn't documented.

I also fixed an issue in the pre-commit hook, and added checkSpaceTest.sh from #32613 to that hook. (Otherwise, there would have been a merge conflict.)

Here's the script I used to create the new .may_include files:

$ cd tor
$ echo '*.h' > .may_include
$ for cf in `find src -name *.c`; do
    d=`dirname $cf`
    if [ ! -e $d/.may_include ]; then
        echo $d/.may_include
    fi
  done | sort -u | grep -v ext | xargs -n1 cp .may_include
$ echo '*.inc' >> src/app/config/.may_include
$ echo '*.inc' >> src/test/.may_include
$ rm .may_include

comment:7 Changed 8 weeks ago by nickm

Resolution: implemented
Status: needs_reviewclosed

LGTM; squashed and merged.

Note: See TracTickets for help on using tickets.