Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

From: Jeff King <peff@peff.net>
Date: Mar 18, 2016, 05:00 UTC
Message-ID: <20160318050017.GA22327@sigill.intra.peff.net>
In-Reply-To: <CAKqreuw7Am_wZQjYYjvsxx0Ccr4OOwoF=EnLvMTK9jxeBUFv5Q@mail.gmail.com>

On Fri, Mar 18, 2016 at 12:34:04PM +0800, 惠轶群 wrote:

Show 15 quoted lines

> >> +test_expect_success 'set $XDG_RUNTIME_DIR' '
> >> +     XDG_RUNTIME_DIR=$HOME/xdg_runtime/
> >> +'
> >
> > Doesn't this need to export the variable so that credential-cache can
> > see it?
> 
> I'm not sure, but it seems that a little clean up code added before
> send-email
> make the test fail. At that time, I run test without building. I've send
> PATCH v2
> which runs well on my computer. However, $XDG_RUNTIME_DIR is still not
> exported, but that just works.
> 
> I will try to dig deeper into the bash script to see why.

I suspect it is because you have $XDG_RUNTIME_DIR defined in your environment, which causes the shell to automatically export it. I don't, so an explicit "export" is required to for the variable to make it to its children.

I think we should actually be unsetting it in test-lib.sh for all tests, as we do for XDG_CONFIG_HOME. That makes sure the tests are running with a known state.

For the non-XDG_RUNTIME_DIR tests, does this mean we are creating the socket in /tmp? I'm not entirely happy with that, as we usually try to have the test suite avoid touching anything outside of its trash directories.

Show 9 quoted lines

> > This runs the full suite of tests twice (once here, and once for the
> > original helper_test invocation you left below). Shouldn't we just do it
> > once (making sure that $XDG_RUNTIME_DIR is respected)?
> 
> I'd like to test the behavior of git-credential-cache when $XDG_RUNTIME_DIR
> is unset.
> 
> In `t/t0302-credential-store.sh`, helper_test is also run multiple times.
> That's why I do so.

OK. My main concern was just that the tests would take too long, but the slow one is the cache test at the end, which is not repeated. So I think this is fine.

Show 13 quoted lines

> > I wondered if this might be racy. credential-cache tells the daemon
> > "exit", then waits for a response or EOF. The daemon sees "exit" and
> > calls exit(0) immediately. We clean up the socket in an atexit()
> > handler. So I think we are OK (the pipe will get closed when the process
> > exits, and the atexit handler must have run by then).
> >
> > But that definitely was not designed, and is just how it happens to
> > work. I'm not sure if it's worth commenting on that (here, or perhaps in
> > the daemon code).
> 
> I'm still confused.
> 
> What do you mean by "pipe"? should it be "socket" instead?

Sorry, yes, I used "pipe" and "socket" interchangeably there.

> What is not designed? cleanup being done, my tests passing or the
> synchronization?

The synchronization. If the daemon were implemented as:

  if (!strcmp(action.buf, "exit")) {
	/* acknowledge that we got command */
	fclose(out);
	exit(0);
  }

for example, then the client would exit at the same that the daemon is cleaning up the socket, and we may or may not call test_path_is_missing before the cleanup is done.

I think it's OK to rely on that, but we may want to put a comment to that effect in the daemon code so that it doesn't get changed.

-Peff