Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions

From: Glen Choo <chooglen@google.com>
Date: Jul 20, 2023, 17:42 UTC
Message-ID: <kl6lzg3qzdhn.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqjzuv5vvg.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 6 quoted lines

> New helper functions that do not have any caller and no
> documentation to explain how they are supposed to be called
> (i.e. the expectation on the callers---what values they need to feed
> as parameters when they call these helpers, and the expectation by
> the callers---what they expect to get out of the helpers once they
> return) makes it impossible to evaluate if they are any good [*].

Agreed.

Show 5 quoted lines

> 	Side note.  Those of you who are keen to add unit tests to
> 	the system (Cc:ed) , do you think a patch line this one that
> 	adds a new helper function to the system, would benefit from
> 	being able to add a few unit tests for these otherwise
> 	unused helper functions?

Absolutely. As a rule, we should strive to test all of our changes as they are introduced. With our current shell-based testing, this means that we have to add callers (either via a builtin or test-helper), but IMO a unit test framework would serve this purpose even better.

> 	The calls to the new functions that the unit test framework
> 	would make should serve as a good piece of interface
> 	documentation, showing what the callers are supposed to pass
> 	and what they expect, I guess.

Agreed, and as documentation, unit tests can be easier to read, since they can include only the relevant details.

> 	So whatever framework we choose, it should allow adding a
> 	test or two to this patch easily, without being too
> 	intrusive.  Would that be a good and concrete evaluation
> 	criterion?

Perhaps, but the biggest blocker to adding a unit tests is whether the source file itself is amenable to being unit tested (e.g. does it depend on global state? does it compile easily?). Once that is in place, I can't imagine that there would be a sensible unit test framework that doesn't make it easy to add tests to a patch like this.