Re: [PATCH] make test: Make failure results more verbose

[Max Nikulin]

Ihor, I have tried your patch. My opinion is that you can go ahead and 
commit it as is. Reaction to my comments is optional.

> Subject: [PATCH] make test: Make failure results more verbose

At first it was not clear to me that only *summary* of test results is 
affected.

> +ifeq ($(BTEST_ERT_VERBOSE),yes)

I would consider (it should be a single line)

ifneq (,$(filter-out 0 n N no No NO f F false False 
FALSE,$(BTEST_ERT_VERBOSE)))

to allow more variants to enable verbose logging, e.g. "Y", "true" or 
"1". The idea is that only empty string "", 0, "n", "no", "f", "false" 
values should disable verbose summary.

On 11/01/2022 23:46, Max Nikulin wrote:
> On 07/01/2022 22:04, Ihor Radchenko wrote:
> > Max Nikulin writes:
> >
> > Also, we can adjust the ERT output using
> > ert-batch-backtrace-right-margin, ert-batch-print-level,
> > ert-batch-print-level, and ert-batch-backtrace-line-length

COLUMNS environment variable may be a sensible default when set. It 
seems, some of variables are quite recent addition to Emacs, so some 
care is required.

Maybe I am just not get used to such form of output, maybe I expect 
pretty print output instead of single form per line, maybe enough of 
call stack entries adds no value (e.g. pytest allows to suppress some 
frames from helper functions by defining special local variable and it 
cuts some outer frames by default). Actually some experience is required 
to quickly extract necessary information from backtrace. Even output of 
Python's pdb looks unfamiliar at first because gdb formats it in another 
way.

> Thank you. After sending that message I realized that I did not try to 
> load just ert.el from git to emacs version that I have installed.

It was more tricky than I expected. Latest ert is incompatible with 
Emacs-26. I am not motivated enough to build it from git, but compiling 
ert for 27 version generates no errors, just some warnings.

> > > In my opinion, code of test should be written having clear error reports
> > > in mind.
>
> > Do you have good ideas what could be changed?

Consider `test-org-element/bold-parser' with a typo. Current summary:

  FAILED  test-org-element/bold-parser  ((should (equal 
(org-element-contents (org-test-with-temp-text "*first line\nsecond 
line*" (org-element-map ... ... ... nil t))) '("first line\nSecond 
line"))) :form (equal (#("first line\nsecond line" 0 22 (:parent (bold 
... #3)))) ("first line\nSecond line")) :value nil :explanation 
(list-elt 0 (array-elt 11 (different-atoms (115 "#x73" "?s") (83 "#x53" 
"?S")))))

After some reorder

  (should
   (equal
    '("Line 1\nline 2")
    (org-test-with-temp-text "*Line 1\nline 2*"
      (org-element-contents
       (org-element-map (org-element-parse-buffer) 'bold #'identity nil 
t)))))

it becomes a bit easy to read since expected value and input are closer 
to beginning:

   FAILED  test-org-element/bold-parser  ((should (equal '("Line 
1\nline 2") (org-test-with-temp-text "*Line 1\nLine 2*" 
(org-element-contents (org-element-map ... ... ... nil t))))) :form 
(equal ("Line 1\nline 2") (#("Line 1\nLine 2" 0 13 (:parent (bold ... 
#3))))) :value nil :explanation (list-elt 0 (array-elt 7 
(different-atoms (108 "#x6c" "?l") (76 "#x4c" "?L")))))

Actually I am a bit surprised that `different-atoms' are shortened to 
"..." in long failure report but readable in summary.

> Side note. In f0c474e659b81da0d2ab75e7ec109355965f7a1c I have noticed
> "* H1\nP1\n<point*H2\n"
> that likely should be
> "* H1\nP1\n<point>* H2\n"

Unrelated to this patch, but still should be fixed.

> > > I am unsure if this line or local.mk has priority. I am unsure the the
> > > following is better as well.
> > > BTEST_ERT_VERBOSE ?= yes
> >
> > I am not very familiar with Makefile conventions. Just followed the
> > existing settings in the same file. All other BTEST_ERT_* settings just
> > use "=".

It was a false alarm, sorry for it. local.mk included after default.mk, 
so there is no problem to override value of BTEST_ERT_VERBOSE.

> > > > +    TMPDIR=$(testdir) EMACS_TEST_VERBOSE=yes $(BTEST)
> > >
> > > A purist would say that it is not a directory, it is something like
> > > ...FLAGS or ...ARGS. I know, it was abused before your patch.
> >
> > I do not follow you here.
> > VARIABLE1=1 VARIABLE2=2 /path/to/script
> > is the usual way to set variables in script environment in bash.
>
> My bad. I did not noticed that it is a part of script of a rule. I 
> believed that the whole line is assignment to TMPDIR variable. I am 
> unsure if the following approach to avoid excessive ifeq's without 
> modification of the rule is better:
>
> ifeq ($(BTEST_ERT_VERBOSE),yes)
> EMACS_TEST_VERBOSE=yes
> export EMACS_TEST_VERBOSE
> endif

With such approach the variable is exported for all targets, not only 
for testing ones, however it will not harm. The advantage of your 
variant is that value of the variable value may be found in logs, 
disadvantage is that makefile is a bit more obscure.

> > > Shouldn't it be mentioned in testing/README?
> >
> > Only BTEST_RE is documented there. BTEST_PRE, BTEST_POST,
> > BTEST_OB_LANGUAGES, and BTEST_EXTRA are not documented.
> > I believe that the odds for the user to change BTEST_ERT_VERBOSE are
> > rather low and it is not worth mentioning it explicitly in README.
> >
> > Or, alternatively, we can document all the settings in README.
> > WDYT?
>
> I agree that default verbose log is preferred in most cases. The only 
> exception when it becomes rather useless and just consumes disk space is 
> something basic is broken (see my comment concerning limit on failures 
> above). Let's assume that you convinced me and only rare developers will 
> customize the value.