From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp12.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id gG7NG8jE4mGyggAAgWs5BA (envelope-from ) for ; Sat, 15 Jan 2022 13:57:44 +0100 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp12.migadu.com with LMTPS id ACy2GMjE4mEbsAAAauVa8A (envelope-from ) for ; Sat, 15 Jan 2022 13:57:44 +0100 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id BC3B4291A4 for ; Sat, 15 Jan 2022 13:57:43 +0100 (CET) Received: from localhost ([::1]:36388 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1n8ico-0004tt-Sz for larch@yhetil.org; Sat, 15 Jan 2022 07:57:42 -0500 Received: from eggs.gnu.org ([209.51.188.92]:54896) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n8iYL-0004t6-7I for emacs-orgmode@gnu.org; Sat, 15 Jan 2022 07:53:11 -0500 Received: from ciao.gmane.io ([116.202.254.214]:47008) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n8iYG-0000iK-3v for emacs-orgmode@gnu.org; Sat, 15 Jan 2022 07:53:04 -0500 Received: from list by ciao.gmane.io with local (Exim 4.92) (envelope-from ) id 1n8iYB-0007zH-JE for emacs-orgmode@gnu.org; Sat, 15 Jan 2022 13:52:55 +0100 X-Injected-Via-Gmane: http://gmane.org/ To: emacs-orgmode@gnu.org From: Max Nikulin Subject: Re: [PATCH] make test: Make failure results more verbose Date: Sat, 15 Jan 2022 19:52:47 +0700 Message-ID: References: <87ee5q5lic.fsf@localhost> <87lezrr3i7.fsf@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 In-Reply-To: Content-Language: en-US Received-SPF: pass client-ip=116.202.254.214; envelope-from=geo-emacs-orgmode@m.gmane-mx.org; helo=ciao.gmane.io X-Spam_score_int: 28 X-Spam_score: 2.8 X-Spam_bar: ++ X-Spam_report: (2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_ADSP_CUSTOM_MED=0.001, FORGED_GMAIL_RCVD=1, FORGED_MUA_MOZILLA=2.309, FREEMAIL_FORGED_FROMDOMAIN=0.248, FREEMAIL_FROM=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.248, NICE_REPLY_A=-0.001, NML_ADSP_CUSTOM_MED=0.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Migadu-Flow: FLOW_IN X-Migadu-Country: US ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1642251463; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=itPLFLSbikPA+xoWn2WEJO7duAj7P2gRrvvAm02monM=; b=qFoW2e0qBTz6XPGXRxzwzpGetspo7iCnwRT3RD5Qg68CHFR6kNzrvtxOKecjEpPZfJ96BD LoM13cMnBduMFb2DYaFbzjTJOTnZcOyWbtY5r7vocNrvQ+x4UDWBhdf9KLd4cI4YrIst9L 2W0s5g83UvcoPshir7mO3IpR1Bwb+dletZe8T/BcByuU6L301oxcLPezsKNrAcsUFQmLD5 N0Ib69xpN6tyzfwdQMn5nDJjt8VYBxYfrgb7B5vpY95GPbUg0rPxk8C0z8VjXVEmLrYr/P r7724n1IQhWgli+TDWQHGD8Q2L95zUuf5jlf2mPhopH0YXI/CL2y2igUmWHQcg== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1642251463; a=rsa-sha256; cv=none; b=ompjMuxBUIthMzUkVih50DtO0pyC33FlnJg52K554lfbJGI7NdcwinK/08VsukgzdAaUPK wp3k7PiSKuRTvNvL6pGOAaxzDyI1Fywu4B/aZG7wFgPu0J8bgZXzvhorA+Fp6phv4J0sDN vf842qsdzDu+Q+1YlTfuPK24B73Q1Zn6rqxVQpUwRPcIlozeJlLgliFSprfgAEhjf4dy6i ZjQJogmjyZi5dhw9avxJGfICOJoOIICLyIP4PcxnzbCElYNsQD9wVwNgSrmH+92LMZ2D3d 1w6GbVIdwalw82MECQU0ZSLtGAeLAARcj4dzAEGtjqIKd5nAex84uj1IotDN2Q== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" X-Migadu-Spam-Score: -3.33 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: BC3B4291A4 X-Spam-Score: -3.33 X-Migadu-Scanner: scn0.migadu.com X-TUID: LozGUxOOZZB9 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 that likely should be > "* H1\nP1\n* 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.