From: Max Nikulin <email@example.com>
Subject: Re: [PATCH] make test: Make failure results more verbose
Date: Sat, 15 Jan 2022 19:52:47 +0700 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
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
> +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
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
> 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"
After some reorder
'("Line 1\nline 2")
(org-test-with-temp-text "*Line 1\nline 2*"
(org-element-map (org-element-parse-buffer) 'bold #'identity nil
it becomes a bit easy to read since expected value and input are closer
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)
> export EMACS_TEST_VERBOSE
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.
> 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.
next prev parent 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 [PATCH] make test: Make failure results more verbose 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
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:
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
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).