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

Glen Choo
2023-07-20T17:42:17Z

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

> 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.

> 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.