Subject: Re: bug#51037: [PATCH] Make `print-level` & `print-length` customizable in ERT batch tests

From: Michael <[email-masked]>
Date: Mon, 15 Nov 2021 15:11:54 -0800

Eli Zaretskii <[email-masked]> writes:

>> From: Michael <[email-masked]>
>> Cc: [email-masked], [email-masked], [email-masked]
>> Date: Sun, 14 Nov 2021 07:42:27 -0800
>> 
>> This commit introduces three new ert variables:
>> ert-batch-print-length, ert-batch-print-level &
>> ert-batch-backtrace-length. The first two control print-length
>                             ^^
> Please leave 2 spaces between sentences, per our coding 
> conventions
> (here and elsewhere in your patch).

Fixed.

>> +line lengths (i.e. to get full backtraces), or a positive 
>> integer to
>
> Either "i.e.," (with a comma), or "i.e.@:".  Otherwise TeX will
> typeset "i.e." as if it ends a sentence.

Fixed.

>> +** New ERT batch variables
>> +
>> +Add three variables 'ert-batch-print-length', 
>> 'ert-batch-print-level'
>
> In NEWS we use a different style, like this:
>
>   Three new variables 'ert-batch-print-length', ...
>
> IOW, "Add" is not appropriate here.

Kindly fixed already by Lars.

>>  Backtrace mode will attempt to abbreviate printing of 
>>  backtrace
>> -frames to make them shorter than this, but success is not
>> -guaranteed.  If set to nil or zero, Backtrace mode will not
>> -abbreviate the forms it prints."
>> +frames by setting `print-level' & `print-length' to make them
>
> Please don't use "&" in doc strings; use "and" instead (here and
> elsewhere in the patch).

Fixed.

>> +shorter than this, but success is not guaranteed.  If set to 
>> nil
>> +or zero, Backtrace mode will not abbreviate the forms it 
>> prints."
>             ^^^^^^^^^
> "backtrace", not capitalized/

Oh! Good catch -- fixed.

>> -(defun ert--setup-results-buffer (stats listener buffer-name)
>> +(defvar ert--output-buffer-name "*ert*")
>> +
>> +(defun ert--setup-results-buffer (stats listener)
>>    "Set up a test results buffer.
>>  
>> -STATS is the stats object; LISTENER is the results listener;
>> -BUFFER-NAME, if non-nil, is the buffer name to use."
>> -  (unless buffer-name (setq buffer-name "*ert*"))
>> -  (let ((buffer (get-buffer-create buffer-name)))
>> +STATS is the stats object; LISTENER is the results listener."
>> +  (let ((buffer (get-buffer-create ert--output-buffer-name)))
>
> Why this change in the signature of the function?

A consequence of a decision discussed elsewhere in this thread:
the only reason that parameter existed was to enable unit
testing, which I re-wrote to use `cl-letf`.

Background: prior to this patch, `ert-run-tests-interactively`
accepted two optional parameters: output-buffer-name and
message-fn. The were soley used for the purpose of unit testing,
and were even documented in commentary as such:

;; Should OUTPUT-BUFFER-NAME and MESSAGE-FN really be arguments 
   here?
;; They are needed only for our automated self-tests at the 
   moment.
;; Or should there be some other mechanism?

I carried over the idiom to ert-run-tests-batch when adding my
own unit tests. Lars pointed out that I could use `cl-letf` to
override the function definition of `message`, rather than using
a "test-only" parameter. I did that, then removed the test-only
parameters from `ert-run-tests-interactively` an re-wrote those
unit tests to also use `cl-letf`. At that point, there was no one
using the the `buffer-name` parameter to
`ert--setup-results-buffer`. Since it's an internal function, I
felt comfortable just removing the parameter.

-- 
Michael <[email-masked]>