On Tue, May 09, 2023 at 11:27:35AM -0700, Glen Choo wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +		git config fetch.parallel 0 &&
> >
> > Is this necessary for the purpose of the test, though?  It should
> > not hurt, but we do not require the end-users to set it in real life
> > for the parallel fetching to work, either, right?
> 
> IIUC it would make the test output deterministic if we fetched from both
> remotes. That doesn't happen here though, so I guess it's not doing
> anything right now.

Right, will drop.

> >> +		git fetch --all --no-recurse-submodules 2>../actual
> >> +	) &&
> >> +
> >> +	cat >expect <<-EOF &&
> >> +	From ../src
> >> +	 * [new branch]      master     -> secondary/master
> >> +	EOF
> >> +	test_cmp expect actual
> >> +'
> >
> > In the context of a series that attempts to introduce a new stable
> > output format for machine consumption, which implies the traditional
> > output can change to match human users' preference, this test feels
> > a bit brittle, but let's wait until the end of the series to judge
> > that.
> 
> I also find it a bit brittle to assert on the whole output when this
> test is about checking that we do not fetch the superproject.
> 
> Is there a reason you didn't go with the "grep for submodule lines"
> approach in the previous tests? If it's about catching regressions, IMO
> your PATCH 2/8 does a good enough job of doing that.
> 
> Wondering out loud, I wonder if it makes sense for us to make a bigger
> distinction between "tests whose purpose is to guard against unexpected
> changes in output" (i.e. snapshot tests) vs "tests that happen to use
> output as a way to assert behavior" (i.e. 'regular' behavioral tests).
> Many GUI app codebases have such a distinction and have different best
> practices around them.

Fair enough, will switch to `! grep "Fetching submodule"`.

Patrick