Junio C Hamano <gitster@xxxxxxxxx> 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.
