Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15344 closed enhancement (fixed)

Integrate tests into automake test suite

Reported by: cypherpunks Owned by: cypherpunks
Priority: Very Low Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: testing, 027-triaged-1-in, SponsorS
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

The ntor, backtrace and zero lengths keys tests are not integrated into the automake test suite. This means these tests not being include in the test results (with PASS/FAIL/etc), clutter up the terminal with their own output, and are potentially not being run parallel.

Child Tickets

Change History (24)

comment:1 Changed 4 years ago by cypherpunks

Owner: set to cypherpunks
Status: newassigned

Is it expected behavior that the backtrace test does not communicate its failure?

for I in range(len(LINES)):
    if matches(LINES[I:], FUNCNAMES):
        print("OK")
        break
else:
    print("BAD")

The snippet above from bt_test.py only prints OK or BAD but does not change its exit code so the make rule where it is called never fails.

comment:2 Changed 4 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.7.x-final

comment:3 Changed 4 years ago by nickm

No, it would be good if it had a nonzero exit code in that case.

comment:4 Changed 4 years ago by nickm

See #11659 for a related ticket.

Changed 4 years ago by cypherpunks

comment:5 Changed 4 years ago by cypherpunks

For documentation purposes, @abs_top_srcdir@ is used deliberately in test_ntor.sh.in and test_bt.sh.in because autoconf ignores the non-recursive nature of Tor. Variable substitutions of relative paths are computed from the directory the substituted files are located in, but tests are executed from the build directory root.
For example, with test_ntor.sh the top_srcdir variable would result in ../../src/test. When executed from the build directory root it is obvious that this relative path won't find the files it is looking for. Therefore we use absolute paths.

comment:6 Changed 4 years ago by nickm

Status: assignedneeds_review

comment:7 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:8 Changed 4 years ago by nickm

I wonder if there's some way to do this without moving all our scripts into getting pre-processed by autoconf.

comment:9 Changed 4 years ago by isabela

Keywords: SponsorS added
Points: small
Priority: minortrivial
Version: Tor: unspecifiedTor: 0.2.7

comment:10 in reply to:  8 Changed 4 years ago by cypherpunks

Replying to nickm:

I wonder if there's some way to do this without moving all our scripts into getting pre-processed by autoconf.

I have not found a way to pipe output from one command to another without using wrapper scripts as in the ntor and backtrace tests. With regards to the zero length test, i agree it is ugly to have the entire script be processed by autoconf.

A solution would be to give the path to the Tor binary as a parameter so it does not need to be hard coded into the script. As for the shell substitution (@SHELL@), it can be removed if the script does not need to call itself anymore. This be achieved by changing some logic within the script. These solutions would still require a wrapper script (similar to the ntor and backtrace tests) that calls the zero length script with the appropriate Tor binary but at least the zero length script itself would not require any pre-processing.

As final remarks, i think we will always need some pre-processing in the tests if we want to support out-of-tree builds properly. The best solution i can currently come up with is to concentrate the pre-processing to the wrapper scripts.

Expect a fixup patch in a couple of days.

comment:11 Changed 4 years ago by cypherpunks

A fixup patch did not apply cleanly so i created a separate branch (hence the new patches). I took this opportunity to rebase the patches on the current master (0693955) and clean them up (there were some unintentional file mode changes). The old patches 0001, 0002 and 0003 are now deprecated, but patch 0004 can be reused as it only adds the changes file.

The new approach for the zero lengths keys test is described in its commit message.

comment:12 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged to master; thanks!

comment:13 Changed 4 years ago by nickm

Resolution: implemented
Status: closedreopened

Hm. Apparently this breaks with older automakes and makes our tests not pass on Debian stable. I should fix that...

comment:14 Changed 4 years ago by nickm

d579d75 may fix this...

comment:15 Changed 4 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Indeed, it seems to do so.

comment:16 Changed 4 years ago by cypherpunks

Sorry for the incomplete patch. I tend to focus on portability but not so much on backwards compatibility. Great fix, i couldn't find the problem myself. However, i have two remarks on your fixes. Do with it what you want, it's nothing major.

  1. commit ecf98313d673f010b52f8d29c352cd9b8f145734 doesn't do anything but make unprocessed files executable. AFAIK it didn't contribute in fixing the problem and smells like bad practice. Therefore, IMO the commit should be reverted.
  2. all of the files generated in the build directory have permissions 600 (or 700 for executable files). For consistency the wrapper scripts should also be 700 (or maybe use chmod u+x).

Just my two (remark) cents.

comment:17 Changed 4 years ago by nickm

Reverted that commit.

Trying it with u+x.

Note: See TracTickets for help on using tickets.