bug#14781: coding system changes cause make check to hang in icalendar tests

bug#14781: coding system changes cause make check to hang in icalendar tests

[Glenn Morris]

Package: emacs
Severity: important
Version: 24.3.50

Current trunk, GNU/Linux, make check is broken by recent changes related
to coding systems (I suspect):

make check
...
Converting iCalendar...done
   passed  122/331  icalendar-import-with-uid
Preparing iCalendar...
Preparing iCalendar...done
Reading iCalendar...
Reading iCalendar...done
Converting iCalendar...
Converting iCalendar...done
Saving file /tmp/icalendar-test-diary13327GoO...
Wrote /tmp/icalendar-test-diary13327GoO
Converting iCalendar...done
Preparing iCalendar...
Preparing iCalendar...done
Reading iCalendar...
Reading iCalendar...done
Converting iCalendar...
Converting iCalendar...done
Saving file /tmp/icalendar-test-diary13327TyU...
Wrote /tmp/icalendar-test-diary13327TyU
Converting iCalendar...done
Preparing iCalendar...
Preparing iCalendar...done
Reading iCalendar...
Reading iCalendar...done
Converting iCalendar...
Converting iCalendar...done
Saving file /tmp/icalendar-test-diary13327g8a...
Wrote /tmp/icalendar-test-diary13327g8a
Converting iCalendar...done
Preparing iCalendar...
Preparing iCalendar...done
Reading iCalendar...
Reading iCalendar...done
Converting iCalendar...
Converting iCalendar...done
Saving file /tmp/icalendar-test-diary13327tGh...
Wrote /tmp/icalendar-test-diary13327tGh
Converting iCalendar...done
Preparing iCalendar...
Preparing iCalendar...done
Reading iCalendar...
Reading iCalendar...done
Converting iCalendar...
Converting iCalendar...done
Saving file /tmp/icalendar-test-diary133276Qn...
Select coding system (default chinese-iso-8bit): 

...hangs waiting for input.

bug#14781: coding system changes cause make check to hang in icalendar tests

[Kenichi Handa]

In article <6zju35ux8.fsf@fencepost.gnu.org>, Glenn Morris <rgm@gnu.org> writes:

> Current trunk, GNU/Linux, make check is broken by recent changes related
> to coding systems (I suspect):

> make check
> ...
> Converting iCalendar...done
>    passed  122/331  icalendar-import-with-uid
[...]
> Saving file /tmp/icalendar-test-diary133276Qn...
> Select coding system (default chinese-iso-8bit): 

> ...hangs waiting for input.

I've just committed a fix to the trunk.  Actually, the
culprit was test/automated/decoder-tests.el which changes
priorities of coding systems.  I didn't know that all tests
under test/automated are executed in a single Emacs session,
and thus they must be side-effect free.  Isn't it better to
execute each test file in its own Emacs session?

---
Kenichi Handa
handa@gnu.org

bug#14781: coding system changes cause make check to hang in icalendar tests

[Glenn Morris]

Kenichi Handa wrote:

> I've just committed a fix to the trunk.

Thanks.

>  Actually, the culprit was test/automated/decoder-tests.el which
> changes priorities of coding systems. I didn't know that all tests
> under test/automated are executed in a single Emacs session, and thus
> they must be side-effect free.

I think they should all be side-effect free (as much as possible)
anyway. Someone might well want to run an individual test in an existing
Emacs session, without that session getting messed up.

> Isn't it better to execute each test file in its own Emacs session?

I don't know. Running them all in one is the only way to get the final
number of tests run / passed / failed to be accurate. Running them in
separate Emacs's would allow them to run in parallel, which would be
faster, but might be confusing?

bug#14781: coding system changes cause make check to hang in icalendar tests

[Stefan Monnier]

> number of tests run / passed / failed to be accurate. Running them in
> separate Emacs's would allow them to run in parallel, which would be
> faster, but might be confusing?

The time to run each test tends to be small, so the overhead of starting
a separate Emacs session for each one may dwarf the potential speed up
from parallel execution, unless you have hundreds of cores.


        Stefan

bug#14781: coding system changes cause make check to hang in icalendar tests

[Handa Kenichi]

In article <dmr4f96lki.fsf@fencepost.gnu.org>, Glenn Morris
<rgm@gnu.org> writes:

> >  Actually, the culprit was test/automated/decoder-tests.el which
> > changes priorities of coding systems. I didn't know that all tests
> > under test/automated are executed in a single Emacs session, and
> > thus
> > they must be side-effect free.

> I think they should all be side-effect free (as much as possible)
> anyway. Someone might well want to run an individual test in an
> existing
> Emacs session, without that session getting messed up.

I generally agree that keeping a test code site-effect free
is a good thing, and it makes testing a test code easier.
But, sometimes achieving side-effect free is difficult, and
in such a case, I think we don't have to spend much effort
on it.  The main purpose of test is to find bugs.

> > Isn't it better to execute each test file in its own Emacs session?

> I don't know. Running them all in one is the only way to get the final
> number of tests run / passed / failed to be accurate.

Here's a rough idea to do that in test/automated/Makefile.in.

TEST_RESULTS = $(patsubst %.el, %.dat, $(wildcard *.el))

.SUFFIXES: .dat .el 

.el.dat:
        @$(emacs) -f ert-run-tests-batch-and-exit-with-report $< $@

check: $(TEST_RESULTS)
        @$(emacs) -f ert-summarize-test-report $(TEST_RESULTS)

Note that ert-run-tests-batch-and-exit-with-report and
ert-report-test-results are not yet implemented.

> Running them in
> separate Emacs's would allow them to run in parallel, which would be
> faster, but might be confusing?

The faster the better, but I think the speed is not that
important here.  The merit of the above method is that we
can run a single test file just by "make XXXX.dat".

---
Kenichi Handa
handa@gnu.org

bug#14781: coding system changes cause make check to hang in icalendar tests

[Glenn Morris]

Handa Kenichi wrote:

> The merit of the above method is that we can run a single test file
> just by "make XXXX.dat".

You can do that now with not too much effort.

emacs -batch -l ./man-tests.el -f ert-run-tests-batch-and-exit

bug#14781: coding system changes cause make check to hang in icalendar tests

[Glenn Morris]

Handa Kenichi wrote:

> TEST_RESULTS = $(patsubst %.el, %.dat, $(wildcard *.el))

Not portable?
(Though TBH I don't know why we don't just require GNU make and make our
lives easier. But that's a separate issue! :) )

bug#14781: coding system changes cause make check to hang in icalendar tests

[Stefan Monnier]

>> TEST_RESULTS = $(patsubst %.el, %.dat, $(wildcard *.el))
> Not portable?
> (Though TBH I don't know why we don't just require GNU make and make our
> lives easier. But that's a separate issue! :) )

Indeed I think we should use GNUmakefile in `test'.  I already made
that change in the `elpa' branch.

I'm not sure about requiring GNU Make to build Emacs out of a tarball,
but for "side operations" it should not be a problem.

        Stefan

running each test file independently in test/automated

[Kenichi Handa]

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

In article <jwvbo6akdj2.fsf-monnier+emacs@gnu.org>, Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> I'm not sure about requiring GNU Make to build Emacs out of a tarball,
> but for "side operations" it should not be a problem.

I tried to implement a code to run each test file in a
different emacs session (and in parallel) assuming that we
can use GNU Make there.  May I commit the attached changes?

---
Kenichi Handa
handa@gnu.org


[-- Attachment #2: for-parallel-test.diff --]
[-- Type: text/x-diff, Size: 8014 bytes --]

=== modified file '.bzrignore'
--- .bzrignore	2013-06-22 02:41:14 +0000
+++ .bzrignore	2013-08-13 11:08:17 +0000
@@ -172,6 +172,8 @@
 src/stamp-h.in
 src/temacs
 test/indent/*.new
+test/automated/*.log
+test/automated/failure-tests
 +*
 src/globals.h
 src/gl-stamp

=== modified file 'ChangeLog'
--- ChangeLog	2013-07-13 00:01:43 +0000
+++ ChangeLog	2013-08-13 11:08:33 +0000
@@ -1,3 +1,8 @@
+2013-08-13  Kenichi Handa  <handa@gnu.org>
+
+	* .bzrignore: Add test/automated/*.log and
+	test/automated/failure-tests.
+
 2013-07-13  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* configure.ac: Simplify --with-file-notification handling.

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2013-07-20 11:51:53 +0000
+++ lisp/ChangeLog	2013-08-13 09:18:54 +0000
@@ -1,3 +1,8 @@
+2013-08-13  Kenichi Handa  <handa@gnu.org>
+
+	* emacs-lisp/ert.el (ert-run-tests-batch-and-exit-single)
+	(ert-summary-report): New functions.
+
 2013-07-20  Kenichi Handa  <handa@gnu.org>
 
 	* international/mule.el (coding-system-iso-2022-flags): Add

=== modified file 'lisp/emacs-lisp/ert.el'
--- lisp/emacs-lisp/ert.el	2013-07-11 16:13:38 +0000
+++ lisp/emacs-lisp/ert.el	2013-08-13 10:54:58 +0000
@@ -31,7 +31,9 @@
 ;; `defun' but defines a test, and `ert-run-tests-interactively',
 ;; which runs tests and offers an interactive interface for inspecting
 ;; results and debugging.  There is also
-;; `ert-run-tests-batch-and-exit' for non-interactive use.
+;; `ert-run-tests-batch-and-exit',
+;; `ert-run-tests-batch-and-exit-single', and `ert-summary-report' for
+;; non-interactive use.
 ;;
 ;; The body of `ert-deftest' forms resembles a function body, but the
 ;; additional operators `should', `should-not' and `should-error' are
@@ -1409,6 +1411,93 @@
           (backtrace))
       (kill-emacs 2))))
 
+;;;###autoload
+(defun ert-run-tests-batch-and-exit-single ()
+  "Like `ert-run-tests-batch-and-exit', but run tests in a single file."
+  ;; It is intended that this function is called in batch mode with a
+  ;; test file as the argument and stderr redirected to a log file.  A
+  ;; specially formated line is written at the end of that log file.
+  ;; That line is parsed by `ert-summary-report' later.
+  (let* ((test-file (pop command-line-args-left))
+	 (compiled (concat (file-name-sans-extension test-file) ".elc"))
+	 (base (file-name-nondirectory test-file))
+	 ;; Some tests (e.g. flymake-tests.el) calls kill-buffer which
+	 ;; may lead to process-kill-buffer-query-function requiring
+	 ;; user iteraction.  Bind this variable to nil avoid that
+	 ;; problem.
+	 (kill-buffer-query-functions nil)
+	 (prefix (format "Running tests in %s..." base)))
+    (unwind-protect
+	(progn
+	  ;; Load a byte-compiled one or TEST-FILE itself.
+	  (if (file-newer-than-file-p compiled test-file)
+	      (progn
+		(setq base (file-name-nondirectory compiled))
+		(load-file compiled))
+	    (let ((buf (find-file-noselect test-file)))
+	      (if (with-current-buffer buf
+		    (and (boundp 'no-byte-compile) no-byte-compile))
+		  (with-current-buffer buf
+		    (eval-buffer))
+		(if (byte-compile-file test-file t)
+		    (setq base (file-name-nondirectory compiled))
+		  (princ (format "%s failed to compile the file\n" prefix))
+		  (message "##REPORT:(compile-error \"%s\")##" base)
+		  (kill-emacs 0)))))
+	  ;; Run tests.
+	  (let* ((stats (ert-run-tests-batch))
+		 (total (ert-stats-total stats))
+		 (expected (ert-stats-completed-expected stats)))
+	    (if (= total expected)
+		(princ (format "%s passed all %d tests\n" prefix total))
+	      (princ (format "%s passed %d tests out of %d\n"
+			     prefix expected total)))
+	    (message "##REPORT:(done %d %d)##" total expected)
+	    (kill-emacs 0)))
+      (princ (format "%s failed to load the file\n" prefix))
+      (message "##REPORT:(load-error \"%s\")##" base)
+      (kill-emacs 0))))
+
+;;;###autoload
+(defun ert-summary-report ()
+  "Print a test report summarizing test log files."
+  ;; It is intended that the function is called after all log files
+  ;; corresponding to test files were generated by
+  ;; ert-run-tests-batch-and-exit-single.
+  (let ((result (cons 0 0))
+	errors)
+    (with-temp-buffer
+      (dolist (logfile command-line-args-left)
+	(erase-buffer)
+	(insert-file-contents logfile)
+	(goto-char (point-max))
+	(or (search-backward "##REPORT:" nil t)
+	    (error "no report line: %s" logfile))
+	(goto-char (match-end 0))
+	(let ((form (read (current-buffer))))
+	  (if (eq (car form) 'done)
+	      (progn
+		(setcar result (+ (car result) (nth 1 form)))
+		(setcdr result (+ (cdr result) (nth 2 form))))
+	    (push form errors)))))
+    (setq command-line-args-left nil)
+    (message "  ## Summary ##")
+    (message "  Ran %s tests, %d results as expected, %d unexpected"
+	     (car result) (cdr result) (- (car result) (cdr result)))
+    (when errors
+      (message "\n  Following test files have problems:")
+      (dolist (err errors)
+	(let* ((tag (car err))
+	       (reason (cond ((eq tag 'emacs-error)
+			      "emacs aborted while processing this file")
+			     ((eq tag 'load-error)
+			      "loading this file failed")
+			     ((eq tag 'compile-error)
+			      "compiling this file failed")
+			     (t
+			      "unknown error"))))
+	  (message "    %s: %s" (nth 1 err) reason)))))
+  (kill-emacs 0))
 
 ;;; Utility functions for load/unload actions.
 

=== modified file 'test/ChangeLog'
--- test/ChangeLog	2013-07-13 01:55:58 +0000
+++ test/ChangeLog	2013-08-13 11:05:23 +0000
@@ -1,3 +1,11 @@
+2013-08-13  Kenichi Handa  <handa@gnu.org>
+
+	* automated/Makefile.in (.SUFFIXES): Add .log.
+	(TEST_LOGS, FAILURE_TESTS, FAILURE_LOGS): New variables.
+	(.el.log, parallel, failure-tests/compile-error.el)
+	(failure-tests/load-error.el, failure-tests/emacs-error.el)
+	(parallel-with-failure-test, clean): New targets.
+
 2013-07-13  Fabián Ezequiel Gallina  <fgallina@gnu.org>
 
 	* automated/python-tests.el (python-imenu-create-index-2)

=== modified file 'test/automated/Makefile.in'
--- test/automated/Makefile.in	2013-06-27 09:26:54 +0000
+++ test/automated/Makefile.in	2013-08-13 06:17:51 +0000
@@ -67,7 +67,7 @@
 # subdirectories, to make sure require's and load's in the files being
 # compiled find the right files.
 
-.SUFFIXES: .elc .el
+.SUFFIXES: .elc .el .log
 
 # An old-fashioned suffix rule, which, according to the GNU Make manual,
 # cannot have prerequisites.
@@ -136,6 +136,41 @@
 	cd $(test); rm -f *.elc */*.elc */*/*.elc */*/*/*.elc
 	$(MAKE) $(MFLAGS) compile EMACS=$(EMACS)
 
+TEST_LOGS = $(patsubst %.el, %.log, $(wildcard $(test)/*.el))
+
+.el.log:
+	@cd $(test); \
+	($(emacs) -f ert-run-tests-batch-and-exit-single $< 2> $@ || \
+	  (BASE="`basename $<`"; \
+	   echo "Running tests in $$BASE... fatal error"; \
+	   echo "##REPORT:(emacs-error \"$$BASE\")###" > $@))
+
+parallel: $(TEST_LOGS)
+	@cd $(test); $(emacs) -f ert-summary-report $(TEST_LOGS)
+
+FAILURE_TESTS = \
+	failure-tests/compile-error.el \
+	failure-tests/load-error.el \
+	failure-tests/emacs-error.el
+
+FAILURE_LOGS = $(patsubst %.el, %.log, $(FAILURE_TESTS))
+
+failure-tests/compile-error.el: $(lastword $(MAKEFILE_LIST))
+	@test -d `dirname "$@"` || mkdir `dirname "$@"`
+	@echo '(ert-deftest test-run-error (should kkk))' > $@
+failure-tests/load-error.el: $(lastword $(MAKEFILE_LIST))
+	@test -d `dirname "$@"` || mkdir `dirname "$@"`
+	@echo 'kkk' > $@
+failure-tests/emacs-error.el: $(lastword $(MAKEFILE_LIST))
+	@test -d `dirname "$@"` || mkdir `dirname "$@"`
+	@echo '(kill-emacs 1)' > $@
+
+parallel-with-failure-test: $(TEST_LOGS) $(FAILURE_LOGS)
+	@cd $(test); $(emacs) -f ert-summary-report $(TEST_LOGS) $(FAILURE_LOGS)
+
+clean:
+	cd $(test); rm -f $(TEST_LOGS) $(FAILURE_TESTS)
+
 bootstrap-clean:
 	cd $(test); rm -f *.elc */*.elc */*/*.elc */*/*/*.elc
 

Re: running each test file independently in test/automated

[Stefan Monnier]

> I tried to implement a code to run each test file in a
> different emacs session (and in parallel) assuming that we
> can use GNU Make there.  May I commit the attached changes?

Sounds OK, but please rename the file to GNUmakefile.


        Stefan

bug#14781: coding system changes cause make check to hang in icalendar tests

[Handa Kenichi]

In article <3ea9lvemla.fsf@fencepost.gnu.org>, Glenn Morris
<rgm@gnu.org> writes:
> Handa Kenichi wrote:
> > TEST_RESULTS = $(patsubst %.el, %.dat, $(wildcard *.el))

> Not portable?
> (Though TBH I don't know why we don't just require GNU make and make
> our
> lives easier. But that's a separate issue! :) )

If portability matters, we can ask the configure script to
generate a proper values:

TEST_RESULTS = %TEST_RESULTS%

> > The merit of the above method is that we can run a single test file
> > just by "make XXXX.dat".

> You can do that now with not too much effort.

> emacs -batch -l ./man-tests.el -f ert-run-tests-batch-and-exit

Not that simple. We must do:

EMACSLOADPATH=../../lisp:. LC_ALL=C ../../src/emacs -batch --no-site-file --no-site-lisp -l ./man-tests.el -f ert-run-tests-batch-and-exit

In article <jwvbo6akdj2.fsf-monnier+emacs@gnu.org>, Stefan Monnier
<monnier@IRO.UMontreal.CA> writes:

> Indeed I think we should use GNUmakefile in `test'.  I already made
> that change in the `elpa' branch.

> I'm not sure about requiring GNU Make to build Emacs out of a tarball,
> but for "side operations" it should not be a problem.

I fully agree.

---
Kenichi Handa
handa@gnu.org

bug#14781: coding system changes cause make check to hang in icalendar tests

[Glenn Morris]

Stefan Monnier wrote:

>> number of tests run / passed / failed to be accurate. Running them in
>> separate Emacs's would allow them to run in parallel, which would be
>> faster, but might be confusing?
>
> The time to run each test tends to be small, so the overhead of starting
> a separate Emacs session for each one may dwarf the potential speed up
> from parallel execution, unless you have hundreds of cores.

I was meaning one Emacs per file, not per test.
I get about a factor of 2 speed-up on 4 cores this way.
The main issue holding it back is the wildly varying length of time that
each test file takes (the file-notify ones are slow for me).