unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / Atom feed
* Tests and linting not coupled to one file (was: Re: master 664927b: Add an expensive test for defcustom types)
       [not found] ` <20200925124647.83150209D4@vcs0.savannah.gnu.org>
@ 2020-09-25 13:11   ` Stefan Kangas
  2020-09-25 13:46     ` Tests and linting not coupled to one file Lars Ingebrigtsen
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Kangas @ 2020-09-25 13:11 UTC (permalink / raw)
  To: Lars Ingebrigtsen, emacs-devel

larsi@gnus.org (Lars Ingebrigtsen) writes:

> branch: master
> commit 664927b5257fdaf26f24063edb1f41c407805ed8
> Author: Lars Ingebrigtsen <larsi@gnus.org>
> Commit: Lars Ingebrigtsen <larsi@gnus.org>
>
>     Add an expensive test for defcustom types
>
>     * admin/cus-test.el (cus-test-opts): Return the tests.
>
>     * test/lisp/custom-tests.el (check-for-wrong-custom-types): Test
>     custom types (bug#30990).

Thanks.

I have been thinking about what to with this kinds of expensive
tests/linting.  It is of course good that we now run the above
automatically, since it will help us catch bugs.  But it isn't very
clean conceptually to add such a test to "custom-tests.el", since it
operates on the entire code base instead of just one file/component.

One idea I've had is to create a new directory "test/misc" or something
to keep such tests in.  We could arrange for them to be run with
"check-expensive" and continue using ert to get standardized reporting,
etc.

The custom tests are good candidates here, but there are others.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Tests and linting not coupled to one file
  2020-09-25 13:11   ` Tests and linting not coupled to one file (was: Re: master 664927b: Add an expensive test for defcustom types) Stefan Kangas
@ 2020-09-25 13:46     ` Lars Ingebrigtsen
  2020-09-25 15:36       ` Stefan Kangas
  0 siblings, 1 reply; 7+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-25 13:46 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: emacs-devel

Stefan Kangas <stefankangas@gmail.com> writes:

> I have been thinking about what to with this kinds of expensive
> tests/linting.  It is of course good that we now run the above
> automatically, since it will help us catch bugs.  But it isn't very
> clean conceptually to add such a test to "custom-tests.el", since it
> operates on the entire code base instead of just one file/component.

Yes, that's true.

> One idea I've had is to create a new directory "test/misc" or something
> to keep such tests in.  We could arrange for them to be run with
> "check-expensive" and continue using ert to get standardized reporting,
> etc.
>
> The custom tests are good candidates here, but there are others.

I think that makes sense, but I wonder whether there's much to be gained
by doing that.  If somebody is looking for "what's that test that checks
all the defcustom declaration", would they look in lisp/custom-tests.el
or misc/defcustom-tests.el?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Tests and linting not coupled to one file
  2020-09-25 13:46     ` Tests and linting not coupled to one file Lars Ingebrigtsen
@ 2020-09-25 15:36       ` Stefan Kangas
  2020-09-26 13:38         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Kangas @ 2020-09-25 15:36 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: emacs-devel

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> One idea I've had is to create a new directory "test/misc" or something
>> to keep such tests in.  We could arrange for them to be run with
>> "check-expensive" and continue using ert to get standardized reporting,
>> etc.
>>
>> The custom tests are good candidates here, but there are others.
>
> I think that makes sense, but I wonder whether there's much to be gained
> by doing that.  If somebody is looking for "what's that test that checks
> all the defcustom declaration", would they look in lisp/custom-tests.el
> or misc/defcustom-tests.el?

That's a fair point.  Likely they would not be aware of "test/misc" at
first.  OTOH we could document it in "test/file-organization.org".

Here's three scenarios where adding a test to the existing files might
not make sense:

- Running `admin/check-doc-strings' automatically.

- Automatic linting using third-party tools.  We don't do that today
  AFAIK, but it might be worth investigating.  I found a relatively long
  list of typos recently using "codespell", and "shellcheck" may or may
  not make sense to use for our shell-scripts.  Just to give two
  examples of reasonably realistic candidates.

- I have a half-baked shell script with some simple heuristics that I've
  used to find like 100 misspelled symbols in doc strings and comments.
  Maybe we would want to run something like it automatically in the
  future.

  (I will push a fix for the typos I found to emacs-27 when I find some
  time.)

That said, if we can find a way to shoe-horn the stuff we need into the
existing test suite without it being overly ugly, it's perfectly fine by
me.  Perhaps that would be less ugly than adding a new "test/misc"
directory.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Tests and linting not coupled to one file
  2020-09-25 15:36       ` Stefan Kangas
@ 2020-09-26 13:38         ` Lars Ingebrigtsen
  2021-02-21 10:37           ` Stefan Kangas
  0 siblings, 1 reply; 7+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-26 13:38 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: emacs-devel

Stefan Kangas <stefankangas@gmail.com> writes:

> That's a fair point.  Likely they would not be aware of "test/misc" at
> first.  OTOH we could document it in "test/file-organization.org".
>
> Here's three scenarios where adding a test to the existing files might
> not make sense:

[...]

> That said, if we can find a way to shoe-horn the stuff we need into the
> existing test suite without it being overly ugly, it's perfectly fine by
> me.  Perhaps that would be less ugly than adding a new "test/misc"
> directory.

There's certainly something to be said for consistency here: If we're
looking for a test of something in foo.el, we look in
lisp/foo-tests.el.  And as you point out, cross-cutting linter stuff
doesn't really fit into that scheme at all.

So upon further consideration, I agree with you that adding a test/misc
directory would be better in the long term.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Tests and linting not coupled to one file
  2020-09-26 13:38         ` Lars Ingebrigtsen
@ 2021-02-21 10:37           ` Stefan Kangas
  2021-02-21 13:18             ` Lars Ingebrigtsen
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Kangas @ 2021-02-21 10:37 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> That's a fair point.  Likely they would not be aware of "test/misc" at
>> first.  OTOH we could document it in "test/file-organization.org".
>>
>> Here's three scenarios where adding a test to the existing files might
>> not make sense:
>
> [...]
>
>> That said, if we can find a way to shoe-horn the stuff we need into the
>> existing test suite without it being overly ugly, it's perfectly fine by
>> me.  Perhaps that would be less ugly than adding a new "test/misc"
>> directory.
>
> There's certainly something to be said for consistency here: If we're
> looking for a test of something in foo.el, we look in
> lisp/foo-tests.el.  And as you point out, cross-cutting linter stuff
> doesn't really fit into that scheme at all.
>
> So upon further consideration, I agree with you that adding a test/misc
> directory would be better in the long term.

(That was in September.)

How does the attached patch look?  It runs all custom tests
automatically - all of them marked as expensive, and two of them marked
as expected to fail.  We will probably want to fix up the failing tests
at some point, too.

[-- Attachment #2: 0001-Run-admin-cus-tests.el-tests-from-test-suite.patch --]
[-- Type: text/x-diff, Size: 11005 bytes --]

From 414e939e3275630f33bdebb90c95bf9f578d93ae Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 19 Feb 2021 12:31:56 +0100
Subject: [PATCH] Run admin/cus-tests.el tests from test suite

* test/Makefile.in (SUBDIRS): Run tests in new directory "misc",
intended for tests not belonging to any one file.
* test/misc/test-custom-deps.el:
* test/misc/test-custom-libs.el:
* test/misc/test-custom-noloads.el:
* test/misc/test-custom-opts.el: New files.
* test/lisp/custom-tests.el (custom--test-local-option): Move test to
above new file test-custom-opts.el.

* admin/cus-test.el: Document running tests from regular test suite.
* test/file-organization.org (Test Files): Document new test directory
"misc" for tests not belonging to any one file.
---
 admin/cus-test.el                |  7 +++++
 test/Makefile.in                 |  2 +-
 test/file-organization.org       |  4 +++
 test/lisp/custom-tests.el        | 11 --------
 test/misc/test-custom-deps.el    | 42 +++++++++++++++++++++++++++++
 test/misc/test-custom-libs.el    | 46 ++++++++++++++++++++++++++++++++
 test/misc/test-custom-noloads.el | 45 +++++++++++++++++++++++++++++++
 test/misc/test-custom-opts.el    | 39 +++++++++++++++++++++++++++
 8 files changed, 184 insertions(+), 12 deletions(-)
 create mode 100644 test/misc/test-custom-deps.el
 create mode 100644 test/misc/test-custom-libs.el
 create mode 100644 test/misc/test-custom-noloads.el
 create mode 100644 test/misc/test-custom-opts.el

diff --git a/admin/cus-test.el b/admin/cus-test.el
index afd5f4ceae..30b5f65561 100644
--- a/admin/cus-test.el
+++ b/admin/cus-test.el
@@ -37,6 +37,13 @@
 ;;
 ;;   src/emacs -batch -l admin/cus-test.el -f cus-test-noloads
 ;;
+;; or as a part of the test suite with
+;;
+;;   make -C test test-custom-opts
+;;   make -C test test-custom-deps
+;;   make -C test test-custom-libs
+;;   make -C test test-custom-noloads
+;;
 ;; in the emacs source directory.
 ;;
 ;; For interactive use: Load this file.  Then
diff --git a/test/Makefile.in b/test/Makefile.in
index ff228d1261..48bbe8712b 100644
--- a/test/Makefile.in
+++ b/test/Makefile.in
@@ -257,7 +257,7 @@ define test_template
 $(foreach test,${TESTS},$(eval $(call test_template,${test})))
 
 ## Get the tests for only a specific directory.
-SUBDIRS = $(sort $(shell find lib-src lisp src -type d ! -path "*resources*" -print))
+SUBDIRS = $(sort $(shell find lib-src lisp misc src -type d ! -path "*resources*" -print))
 
 define subdir_template
   .PHONY: check-$(subst /,-,$(1))
diff --git a/test/file-organization.org b/test/file-organization.org
index 7cf5b88d6d..d1f92da432 100644
--- a/test/file-organization.org
+++ b/test/file-organization.org
@@ -43,6 +43,10 @@ Similarly, tests of features implemented in C should reside in
 ~-tests.el~ added to the base-name of the tested source file.  Thus,
 tests for ~src/fileio.c~ should be in ~test/src/fileio-tests.el~.
 
+Some tests do not belong to any one particular file.  Such tests
+should be put in the ~misc~ directory and be given a descriptive name
+that does /not/ end with ~-tests.el~.
+
 There are also some test materials that cannot be run automatically
 (i.e. via ert).  These should be placed in ~/test/manual~; they are
 not run by the "make check" command and its derivatives.
diff --git a/test/lisp/custom-tests.el b/test/lisp/custom-tests.el
index 10854c71d5..09f79c1a08 100644
--- a/test/lisp/custom-tests.el
+++ b/test/lisp/custom-tests.el
@@ -145,17 +145,6 @@ custom-test-show-comment-preserves-changes
                                (widget-apply field :value-to-internal origvalue)
                                "bar"))))))
 
-(defconst custom-test-admin-cus-test
-  (expand-file-name "admin/cus-test.el" source-directory))
-
-(declare-function cus-test-opts custom-test-admin-cus-test)
-
-(ert-deftest check-for-wrong-custom-types ()
-  :tags '(:expensive-test)
-  (skip-unless (file-readable-p custom-test-admin-cus-test))
-  (load custom-test-admin-cus-test)
-  (should (null (cus-test-opts t))))
-
 (ert-deftest custom-test-enable-theme-keeps-settings ()
   "Test that enabling a theme doesn't change its settings."
   (let* ((custom-theme-load-path `(,(ert-resource-directory)))
diff --git a/test/misc/test-custom-deps.el b/test/misc/test-custom-deps.el
new file mode 100644
index 0000000000..f072adddcb
--- /dev/null
+++ b/test/misc/test-custom-deps.el
@@ -0,0 +1,42 @@
+;;; test-custom-deps.el --- Test custom deps  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2021 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; The command `cus-test-deps' loads all (!) custom dependencies and
+;; reports about load errors.
+
+;;; Code:
+
+(require 'ert)
+
+(defconst custom-test-admin-cus-test
+  (expand-file-name "admin/cus-test.el" source-directory))
+
+(declare-function cus-test-deps custom-test-admin-cus-test)
+(defvar cus-test-deps-errors)  ; from admin/cus-tests.el
+
+(ert-deftest test-custom-deps ()
+  :tags '(:expensive-test)
+  (skip-unless (file-readable-p custom-test-admin-cus-test))
+  (load custom-test-admin-cus-test)
+  (cus-test-deps)
+  (should-not cus-test-deps-errors))
+
+;;; test-custom-deps.el ends here
diff --git a/test/misc/test-custom-libs.el b/test/misc/test-custom-libs.el
new file mode 100644
index 0000000000..70f043d129
--- /dev/null
+++ b/test/misc/test-custom-libs.el
@@ -0,0 +1,46 @@
+;;; test-custom-libs.el --- Test custom loads  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2021 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; This file runs for all libraries with autoloads separate emacs
+;; processes of the form "emacs -batch -l LIB".
+
+;;; Code:
+
+(require 'ert)
+
+(defconst custom-test-admin-cus-test
+  (expand-file-name "admin/cus-test.el" source-directory))
+
+(declare-function cus-test-libs custom-test-admin-cus-test)
+(defvar cus-test-libs-errors)  ; from admin/cus-tests.el
+
+;; FIXME: Currently fails for:
+;;        - lisp/term/ns-win.el
+;;        - lisp/org/org-num.el
+(ert-deftest test-custom-libs ()
+  :tags '(:expensive-test)
+  :expected-result :failed ; FIXME: See above.
+  (skip-unless (file-readable-p custom-test-admin-cus-test))
+  (load custom-test-admin-cus-test)
+  (cus-test-libs t)
+  (should-not cus-test-libs-errors))
+
+;;; test-custom-deps.el ends here
diff --git a/test/misc/test-custom-noloads.el b/test/misc/test-custom-noloads.el
new file mode 100644
index 0000000000..e999fe2abb
--- /dev/null
+++ b/test/misc/test-custom-noloads.el
@@ -0,0 +1,45 @@
+;;; test-custom-deps.el --- Test custom noloads  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2021 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; The command `cus-test-noloads' returns a list of variables which
+;; are somewhere declared as custom options, but not loaded by
+;; `custom-load-symbol'.
+
+;;; Code:
+
+(require 'ert)
+
+(defconst custom-test-admin-cus-test
+  (expand-file-name "admin/cus-test.el" source-directory))
+
+(declare-function cus-test-noloads custom-test-admin-cus-test)
+(defvar cus-test-vars-not-cus-loaded)  ; from admin/cus-tests.el
+
+;; FIXME: Multiple failures here.
+(ert-deftest custom-test-load ()
+  :tags '(:expensive-test)
+  :expected-result :failed ; FIXME: See above.
+  (skip-unless (file-readable-p custom-test-admin-cus-test))
+  (load custom-test-admin-cus-test)
+  (cus-test-noloads)
+  (should-not cus-test-vars-not-cus-loaded))
+
+;;; test-custom-deps.el ends here
diff --git a/test/misc/test-custom-opts.el b/test/misc/test-custom-opts.el
new file mode 100644
index 0000000000..fa6b9e66ae
--- /dev/null
+++ b/test/misc/test-custom-opts.el
@@ -0,0 +1,39 @@
+;;; test-custom-opts.el --- Test custom opts  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2021 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; The command `cus-test-opts' tests many (all?) custom options.
+
+;;; Code:
+
+(require 'ert)
+
+(defconst custom-test-admin-cus-test
+  (expand-file-name "admin/cus-test.el" source-directory))
+
+(declare-function cus-test-opts custom-test-admin-cus-test)
+
+(ert-deftest check-for-wrong-custom-opts ()
+  :tags '(:expensive-test)
+  (skip-unless (file-readable-p custom-test-admin-cus-test))
+  (load custom-test-admin-cus-test)
+  (should (null (cus-test-opts t))))
+
+;;; test-custom-opts.el ends here
-- 
2.30.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Tests and linting not coupled to one file
  2021-02-21 10:37           ` Stefan Kangas
@ 2021-02-21 13:18             ` Lars Ingebrigtsen
  2021-02-21 19:34               ` Stefan Kangas
  0 siblings, 1 reply; 7+ messages in thread
From: Lars Ingebrigtsen @ 2021-02-21 13:18 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: emacs-devel

Stefan Kangas <stefankangas@gmail.com> writes:

> How does the attached patch look?  It runs all custom tests
> automatically - all of them marked as expensive, and two of them marked
> as expected to fail.  We will probably want to fix up the failing tests
> at some point, too.

Looks good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Tests and linting not coupled to one file
  2021-02-21 13:18             ` Lars Ingebrigtsen
@ 2021-02-21 19:34               ` Stefan Kangas
  0 siblings, 0 replies; 7+ messages in thread
From: Stefan Kangas @ 2021-02-21 19:34 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: emacs-devel

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> How does the attached patch look?  It runs all custom tests
>> automatically - all of them marked as expensive, and two of them marked
>> as expected to fail.  We will probably want to fix up the failing tests
>> at some point, too.
>
> Looks good to me.

Thanks, pushed to master as commit 2790c6a572.



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-02-21 19:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200925124646.29315.84502@vcs0.savannah.gnu.org>
     [not found] ` <20200925124647.83150209D4@vcs0.savannah.gnu.org>
2020-09-25 13:11   ` Tests and linting not coupled to one file (was: Re: master 664927b: Add an expensive test for defcustom types) Stefan Kangas
2020-09-25 13:46     ` Tests and linting not coupled to one file Lars Ingebrigtsen
2020-09-25 15:36       ` Stefan Kangas
2020-09-26 13:38         ` Lars Ingebrigtsen
2021-02-21 10:37           ` Stefan Kangas
2021-02-21 13:18             ` Lars Ingebrigtsen
2021-02-21 19:34               ` Stefan Kangas

unofficial mirror of emacs-devel@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/emacs-devel/0 emacs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 emacs-devel emacs-devel/ https://yhetil.org/emacs-devel \
		emacs-devel@gnu.org
	public-inbox-index emacs-devel

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.devel
	nntp://news.gmane.io/gmane.emacs.devel


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git