Re: [BUG/WIP PATCH] unit-tests: use clean test environment

[Patrick Steinhardt <ps@pks.im>]

On Tue, Mar 04, 2025 at 02:30:10AM -0500, Jeff King wrote:
> On Mon, Mar 03, 2025 at 06:07:29AM -0800, Junio C Hamano wrote:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > Maybe. I guess for unit tests it's a lot less clear cut as most of the
> > > tests won't depend on such a controlled environment. So sanitizing the
> > > environment would be a good enough first step for me, and if we see
> > > demand for making specific information available to lots of tests we
> > > could still start to expose those at a later point.
> > 
> > Fair enough.
> > 
> > To put it another way, if you write a test and if it gets affected
> > by externalities, perhaps you are testing a function that is at too
> > high a level that is not a suitable target for unit tested?
> 
> I think one problem with this approach is that breakage is likely going
> to depend on the user's environment. So something that works just fine
> for you, the test author, may introduce a hidden dependency that breaks
> for somebody else much later.
> 
> Some examples, assuming we just suppress reading Git config:
> 
>   - Without an explicit ident, we fall back to constructing one from
>     system info. So if a unit test ever creates a commit, it will work
>     fine for most people, but not for somebody with a blank GECOS field
>     in /etc/passwd. (We do look at that field for reflogs, which current
>     unit tests already do, but we are more forgiving there since we
>     don't pass IDENT_STRICT).
> 
>   - Other programs we call (e.g., imagine gpg or ssh for commit signing
>     or verification) may read their own config based on $HOME,
>     $XDG_CONFIG_HOME, etc. I don't know if Patrick was including that in
>     "sanitizing the environment" or not.

Oh, yes. I didn't mean to say we shouldn't sanitize at all, I rather
meant to say we should sanitize to values that simply cause us to do a
no-op in the relevant parts. That means we'd:

    - Unset a bunch of environment variables where we know that they
      impact Git.

    - Set config-related environment variables to read configuration
      from "/dev/null".

This is in contrast to the more involved fix here, which would be to
populate a temporary home directory with gitconfig files and whatnot.

Patrick