Re: [PATCH v2 01/18] maintenance: create basic maintenance runner

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 4, 2020, 16:32 UTC
Message-ID: <2b6da345-d004-1d83-78cd-3f1ceaf278d9@gmail.com>
In-Reply-To: <20200804144208.GA227292@google.com>

On 8/4/2020 10:42 AM, Jonathan Nieder wrote:

Show 13 quoted lines

> Derrick Stolee wrote:
> 
>> What is seems like you are asking instead is for me to create a tool
>> in the test suite that parses each JSON line, extracts a specific
>> member from that JSON object, reconstructs a command-line invocation
>> from the JSON array, and reports whether that process worked for any
>> line in the event output.
> 
> No, that isn't what I'm asking.
> 
> I'm asking for this patch series to take the existing "grep" lines
> and put them in a function in test-lib-functions.sh, so that we can
> change them in one place when the trace emitter changes.

Thanks for clarifying, clearing up my misinterpretation of your request. I'm coming around to this idea.

Show 10 quoted lines

> [...]
>> If this is to truly be a hard requirement for these tests to move
>> forward,
> 
> Yes, from my point of view it really is.
> 
> But that "is this truly a hard requirement?" comes up tells me I have
> not done a good job of communicating in this review.  A review is
> about participants in the project working together to improve a patch,
> not people making demands at each other.

It's also about a balance. A patch author and a reviewer can have disagreements, and it is important to discover exactly how hard each is holding to their opinion.

Show 6 quoted lines

> [...]
>> If I'm to spend time engineering something more complicated just to
>> check "did this subcommand run with these arguments?" then
> 
> I don't see why this is more complicated than what is in patch 1.  In
> fact, I think it would be a little more simple.

My interpretation (parsing JSON) _was_ more complicated, hence my reservations. The helper still requires non-trivial script-fu (for someone like me who is bad at scripting languages) but centralizing the grep has value.

Here is my attempt so far:

trace2_subcommand_data () {
	command="$1" &&
	commas=$(echo $command | sed s/\ /\",\"/g) &&
	printf "[\"$commas\"]"
}

test_subcommand_called () {
	data=$(trace2_subcommand_data $1) &&
	grep $data $2
}

test_subcommand_not_called () {
	! test_subcommand_called $1 $2
}

with callers looking like

	test_subcommand_called "gc" run-no-auto.txt &&
	test_subcommand_not_called "gc --auto" run-auto.txt &&
	test_subcommand_called "gc --quiet" run-quiet.txt

Thanks, -Stolee