Opened 4 weeks ago

Closed 4 weeks ago

#25560 closed defect (fixed)

test all rust crates for realsies

Reported by: isis Owned by:
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.3-alpha
Severity: Normal Keywords: tor-testing, rust, 033-must
Cc: Actual Points:
Parent ID: Points: .2
Reviewer: nickm Sponsor: SponsorM-can

Description

After we merged #23881, it seems the tests in the tor_log crate aren't being run because there's an array in src/test/test_rust.sh that we didn't add the crate name to. I propose making that shell script automatically find the crates available and run their tests, e.g.

manifests=`find ${abs_top_srcdir:-.}/src/rust/ -mindepth 2 -maxdepth 2 -name 'Cargo.toml'`

(Currently, we're only running tests for crates="protover tor_util smartlist tor_allocate".)

As a side-note, I also think we should be calling cargo with --verbose so that our test logs actually tell us when something's wrong, e.g. for #24795 it would help.

Child Tickets

Change History (8)

comment:1 Changed 4 weeks ago by isis

Priority: MediumHigh
Status: newneeds_review

Patch in my bug25560 branch. Bumping priority up a bit because we're not actually running tests and so we might not be catching bugs.

comment:2 Changed 4 weeks ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

This looks plausible to me. Two issues to check, and I'll be happy.

1) Is this happy under travis, or will we have failures? (I'm checking this now.)

2) Will the "find" line give us correct results when there are spaces in abs_top_srcdir?

comment:3 in reply to:  2 ; Changed 4 weeks ago by isis

Replying to nickm:

This looks plausible to me. Two issues to check, and I'll be happy.

1) Is this happy under travis, or will we have failures? (I'm checking this now.)


It's failing Clang builds but it looks like it's for the same reason as #23881.

2) Will the "find" line give us correct results when there are spaces in abs_top_srcdir?


Oh nope, that'll probably fail miserably, good catch.

comment:4 in reply to:  3 Changed 4 weeks ago by isis

Replying to isis:

Replying to nickm:

2) Will the "find" line give us correct results when there are spaces in abs_top_srcdir?


Oh nope, that'll probably fail miserably, good catch.


Okay, should be fixed with some double-quotes now; same branch.

comment:5 Changed 4 weeks ago by Hello71

that won't fix it; the *output* of find will still contain spaces. the pure POSIX solution would be to use find -exec cargo .... as an aside, `` is obsolete, you should use $(). that doesn't fix the whitespace issue though.

in terms of the actual bug here, though, as I said on IRC, it seems like it would be easier to just use --all as I do in #25386.

comment:6 Changed 4 weeks ago by nickm

Milestone: Tor: 0.3.3.x-final

Right, we need to handle the case. Let's make sure that we're actually testing this in a checkout that's in a directory in a space with it, or else some poor Windows user will be the first person to debug this.

comment:7 Changed 4 weeks ago by isis

Okay, quoting directories is actually fixed now, I think. Surprisingly, building in directories with spaces in their names was/is already broken, because src/test/include.am does this:

test-rust:
        $(TESTS_ENVIRONMENT) $(abs_top_srcdir)/src/test/test_rust.sh

which for me expanded to:

export PYTHON="python"; export SHELL="/bin/bash"; export abs_top_srcdir="/home/isis/code/torproject/foo bar/../tor"; export abs_top_builddir="/home/isis/code/torproject/foo bar"; export builddir="."; export TESTING_TOR_BINARY="./src/or/tor"; export CARGO="cargo"; export CARGO_ONLINE=""; /home/isis/code/torproject/foo bar/../tor/src/test/test_rust.sh

So I've quoted it in the Makefile as well and everything seems to work now.

Let me know if you want me to squash first.

comment:8 Changed 4 weeks ago by nickm

Resolution: fixed
Status: needs_revisionclosed

rebased and squashed on 0.3.3, merged forward!

Please let me know if I messed anything up

Note: See TracTickets for help on using tickets.