Re: [PATCH] Portability: returning void

From: Jeff King <peff@peff.net>
Date: Mar 30, 2011, 04:13 UTC
Message-ID: <20110330041339.GA26281@sigill.intra.peff.net>
In-Reply-To: <20110330035733.GA2793@elie>

On Tue, Mar 29, 2011 at 10:57:33PM -0500, Jonathan Nieder wrote:

Show 21 quoted lines

> Jeff King wrote:
> 
> > While we're at it, let's make the "kill" process a little
> > more robust. Specifically:
> >
> >   1. Wrap the "kill" in a test_when_finished, since we want
> >      to clean up the process whether the test succeeds or not.
> >
> >   2. The "kill" is part of our && chain for test success. It
> >      probably won't fail, but it can if the process has
> >      expired before we manage to kill it. So let's mark it
> >      as OK to fail.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Actually, my (2) above is unlikely to trigger, since the test would have
> > to hang for 100 seconds first, which probably means it is failing
> > anyway. But I did run across it when I screwed up my fix.
> 
> That is actually how the tests distinguish between success and
> failure.  Any idea about a better way?

Ah. I was trying not to look too hard at how the tests actually work, so I completely missed that. Yes, if the "kill" is part of the actual test, then it should stay in the test. We can also put in a test_when_finished to kill it should the test fail to make it that far. If the cleanup one does an extra kill, it shouldn't be a big deal.

> I was in the process of writing a commit message for the same "exec"
> trick, but I'm glad you got to it first. ;-)

I don't know why I didn't think of it sooner. :)

Show 7 quoted lines

> > Also, is it just me, or does it seem weird that test_when_finished
> > blocks failing can produce test failure? I would think you would be able
> > to put any old cleanup crap into them and not care whether it worked or
> > not.
> 
> I'm generally happy that it catches mistakes in cleanup code, but I
> could easily be convinced to change it anyway.

I don't think it's a big deal. It just surprised me.

Show 10 quoted lines

> Maybe it would be good to factor out a helper to make future
> improvements a little easier, like so:
> 
> -- 8< --
> Subject: t0081: introduce helper to fill a pipe in the background
> 
> The fill_input function generates a fifo and runs a command to write
> to it and wait a while.  The intended use is to test programs that
> need to be able to cope with input of limited length without relying
> on EOF or SIGPIPE to detect its end.

Yeah, that looks much nicer. Do you want to just re-roll that on top of what's in 'master', with the "exec" magic and the defensive test_when_finished in it (or as a separate patch on top if you want to split the refactor and fix)? Feel free to borrow from my commit message.

-Peff