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

[Esteban Manchado VelÃzquez <emanchado@xxxxxxxxxxxx>]

On Sun, 12 Feb 2012 04:28:38 +0100, Nick Mathewson <nickm@xxxxxxxxxxxx>  
wrote:

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

   In this concrete case, how would you separate them? One branch per test  
function? Which in this case is more or less one branch per commit?

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

   Yeah, I thought of that too late.

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

   OK.

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

   Yes, that was my idea. Cool that works for you :-)

> > [...] 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.

   Yes, it seems it was only 3 after all. I didn't know what component to  
use so I left it blank. If there's any better way to file them (component,  
Cc or whatever), I'd be happy to learn that and use it in the future :-)

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

   Yeah, "convention" was a strong word. It was just my impression that  
there were more "expected, actual" than "actual, expected", and I thought  
forcing a convention would be good. Although, as Steven said, tinytest  
doesn't actually specify "expected" an "actual" in the output, I find it's  
less "cognitive load" to debug or make sense of tests if you can assume  
what's the order of the values in the failure output.

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

   Oh, hehe, oops. I found test_eq much more readable so I thought that  
was the preferred way.

   I can turn everything into tt_* calls and add a comment to test.h  
stating that test_eq and such are deprecated. Should I do that? In that  
case, however, I would place the expected at the end, because otherwise  
the code will look like "tt_int_op(0, ==, strlen(foo))", which I think  
looks pretty awkward.

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

   OK. I *did* remove a couple of assertions, either because I didn't  
think they added anything, or because they were replaced by (in my view)  
better assertions. In particular, I removed the hardcoded gzip magic  
number check (there's already a check_compression_method, see commit  
c4c1d56d96623a45775ec2544c0c6951fbfa2d9f) and I changed some of the  
strcasecmpend cases (commit 03876f0a721ced6ffebb0c61134d5b8396d7600e).  
There are probably others.

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

   Good catch.

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

   Right, my bad.

> > 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!

   I went through the commits and I definitely have enough to write about.  
I'll send the link when I'm done.

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