Re: [tor-dev] First test "report"

[Nick Mathewson <nickm@xxxxxxxxxxxx>]

On Sat, Feb 11, 2012 at 7:12 AM, Esteban Manchado Velázquez
<emanchado@xxxxxxxxxxxx> wrote:
> Hi there,
>
> I'm done with the first batch of work on the test side. You have the
> (rebased just now) work here:
> https://github.com/emanchado/tor/commits/master.

A suggestion: In the future, it's best to do commits on one or more
"topic branches", where each branch is for a separate kind of work.
That way, it's way easier for upstream to merge some of the commits,
hold off on others, and decline others.

As it stands, if you do all your commits in a "master" branch, and I
want to take some but not all of them, I have to cherry-pick the
individual commits.  Worse still, your branch and the upstream branch
will then have diverged: if you try to pull the official repository
onto your master again, you won't have the actual history of the Tor
master branch , but some other thing that only exists on your master
branch.  This can make stuff yucky fast.

For now, let's leave the current branches as they are.  Once we've got
the contents of your current master branch reviewed/merged/not-merged,
you can reset your master to match tor's, and then do future work in
topic branches.

All that said: I like the granularity of your commits!  Each one is
logically independent and easy to review.

>  The rest of this e-mail
> summarises what I've done and explains my plan for further work.
>
> The content of the commits
> --------------------------
> I have reviewed the whole test_util.c and made three kinds of changes:
>
> 1. Added more tests. Some of them failed, and after checking with other
> people on IRC the conclusion seems to be that they should pass (ie. they
> reproduce actual bugs in the code). To keep the test suite from failing,
> they are inside "#if 0" blocks. So someone should look for "#if 0" inside
> test_util.c and fix the code that makes those fail. Or maybe I should file
> bugs for those?

Filing bugs is the right move; it looks like you've already started to do this.

>
> 2. In comparison assertions, the general convention seems to be to place the
> expected value first ("test_eq(0, functioncall(...))" rather than
> "test_eq(functioncall(...), 0)"). I have modified the assertions not
> following that convention, so they all look the same.

Hm.  I don't think we actually had a convention on this one.

> 3. General clean up, small code reorganisations, fix typos and such. Eg, I
> have turned all the "tt_int_op(a, ==, b)" into "test_eq(a, b)".

Actually, test_eq was the old way; tt_int_op is the newer way since we
switched to tinytest.

Some other comments:

 In general, the hardest thing for me to review here is not whether
the tests are right, but whether they removed any old tests in
revising them.  I'll need to have another look through the patch
series to be sure.

 On commit 5740e0fc1f00fa91be107ee6c4315d114c5ffdc4, the snprintf()
calls  there should be tor_snprintf().

 On commit f40c04a2137724f7b285e8d69ee62e47df1f9049, "iff" is not a
typo.  It is a standard abbreviation for "if and only if."  We use it
to say things like "Return true iff X", since otherwise we would need
to say "Return true if X; return false otherwise."  (If we just said
"Return true if X," the function would technically be allowed to
_always_ return true.)

> I was thinking of blogging about what I saw (esp. related to point 1). I
> think there are valuable lessons to be learned, which will help other people
> writing tests (both for Tor and outside of Tor). I'm not sure if there's
> enough content for a blog post, but if I do it after all, should I post the
> link here?

Please do!

peace,
-- 
Nick
_______________________________________________
tor-dev mailing list
tor-dev@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev