emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Max Nikulin <manikulin@gmail.com>
To: emacs-orgmode@gnu.org
Subject: Re: [PATCH] make test: Make failure results more verbose
Date: Sat, 15 Jan 2022 19:52:47 +0700	[thread overview]
Message-ID: <srug31$t9s$1@ciao.gmane.io> (raw)
In-Reply-To: <srkc8i$elp$1@ciao.gmane.io>

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.



  reply	other threads:[~2022-01-15 12:57 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-02 13:12 Ihor Radchenko
2022-01-03  5:35 ` Max Nikulin
2022-01-07 15:04   ` Ihor Radchenko
2022-01-11 16:46     ` Max Nikulin
2022-01-15 12:52       ` Max Nikulin [this message]
2022-01-15 13:06         ` Max Nikulin
2022-01-15 15:58           ` Max Nikulin
2022-01-21 13:33             ` Ihor Radchenko
2022-01-21 15:01               ` Max Nikulin
2022-01-23 13:31                 ` Ihor Radchenko
2022-01-21 13:31         ` Ihor Radchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='srug31$t9s$1@ciao.gmane.io' \
    --to=manikulin@gmail.com \
    --cc=emacs-orgmode@gnu.org \
    --subject='Re: [PATCH] make test: Make failure results more verbose' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).