bug#23625: viper-tests.el fails in master

bug#23625: viper-tests.el fails in master

[Paul Eggert]

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

Phillip, I just now merged emacs-25 into master and fixed all the merge 
conflicts that were easy, but one problem remains: viper-tests, which 
you recently introduced to emacs-25, fails in the new master. Could you 
please look into this? I don't offhand know what viper-tests is supposed 
to do and don't know how to interpret the test failures. Thanks.

The symptoms with Emacs master commit 
9a834970e822db10bc887db343fd8561a3639a66 compiled for Fedora 23 x86-64, 
are that 'make check' fails with the attached output seemingly being the 
relevant part of the output. This attachment contains some control 
characters; I hope they survive the email process.

(Is there any way to fix the test so that it outputs ordinary text when 
it fails?)


[-- Attachment #2: viper-test-log.txt --]
[-- Type: text/plain, Size: 5866 bytes --]

make[3]: Entering directory '/home/eggert/src/gnu/emacs/static-checking/test'
Compiling lisp/emulation/viper-tests.el
make[3]: Leaving directory '/home/eggert/src/gnu/emacs/static-checking/test'
Testing lisp/emulation/viper-tests.elc
Running 6 tests (2016-05-26 13:26:25-0700)
Loading viper...
Loading /tmp/viper-tests22411xv1...

Test viper-test-fix backtrace:
  viper--deactivate-advice-list()
  viper-go-away()
  toggle-viper-mode()
  #[nil "\301 \210\30!\207" [before-buffer toggle-viper-mode switch-
  viper-test-undo-kmacro([])
  apply(viper-test-undo-kmacro [])
  #[nil "\304\305C\x18\x19\306\x1a\307^[\310\216\311\"\211\x12)\204\x19\0\312\v!\210)\
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test viper-test-fix "Test that the viper
  ert-run-or-rerun-test([cl-struct-ert--stats (not (tag :expensive-tes
  ert-run-tests((not (tag :expensive-test)) #[385 "\306\x02\307\"\203G\0\2
  ert-run-tests-batch((not (tag :expensive-test)))
  ert-run-tests-batch-and-exit((not (tag :expensive-test)))
  eval((ert-run-tests-batch-and-exit (quote (not (tag :expensive-test)
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emulation/viper-test
  command-line()
  normal-top-level()
Test viper-test-fix condition:
    (wrong-number-of-arguments
     (2 . 2)
     1)
   FAILED  1/6  viper-test-fix
   passed  2/6  viper-test-go
undo!
Test viper-test-undo-1 backtrace:
  viper--deactivate-advice-list()
  viper-go-away()
  toggle-viper-mode()
  #[nil "\301 \210\30!\207" [before-buffer toggle-viper-mode switch-
  viper-test-undo-kmacro([97 49 escape 111 50 escape 117])
  #[nil "\304\305\306\307!D\x18\x19\310\x1a\311^[\312\216\313\"\211\x12)\204\x1c\0\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test viper-test-undo-1 "Test for VI like
  ert-run-or-rerun-test([cl-struct-ert--stats (not (tag :expensive-tes
  ert-run-tests((not (tag :expensive-test)) #[385 "\306\x02\307\"\203G\0\2
  ert-run-tests-batch((not (tag :expensive-test)))
  ert-run-tests-batch-and-exit((not (tag :expensive-test)))
  eval((ert-run-tests-batch-and-exit (quote (not (tag :expensive-test)
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emulation/viper-test
  command-line()
  normal-top-level()
Test viper-test-undo-1 condition:
    (wrong-number-of-arguments
     (2 . 2)
     1)
   FAILED  3/6  viper-test-undo-1
Deleted 2 characters
Deleted 2 characters
undo!
Test viper-test-undo-2 backtrace:
  viper--deactivate-advice-list()
  viper-go-away()
  toggle-viper-mode()
  #[nil "\301 \210\30!\207" [before-buffer toggle-viper-mode switch-
  viper-test-undo-kmacro([105 49 32 50 32 51 32 52 32 53 escape 70 50
  #[nil "\304\305\306\307!D\x18\x19\310\x1a\311^[\312\216\313\"\211\x12)\204\x1c\0\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test viper-test-undo-2 "Test for VI like
  ert-run-or-rerun-test([cl-struct-ert--stats (not (tag :expensive-tes
  ert-run-tests((not (tag :expensive-test)) #[385 "\306\x02\307\"\203G\0\2
  ert-run-tests-batch((not (tag :expensive-test)))
  ert-run-tests-batch-and-exit((not (tag :expensive-test)))
  eval((ert-run-tests-batch-and-exit (quote (not (tag :expensive-test)
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emulation/viper-test
  command-line()
  normal-top-level()
Test viper-test-undo-2 condition:
    (wrong-number-of-arguments
     (2 . 2)
     1)
   FAILED  4/6  viper-test-undo-2
Deleted 2 characters
Deleted 2 characters
Deleted 2 characters
Deleted 2 characters
undo!
undo more!
undo more!
Test viper-test-undo-3 backtrace:
  viper--deactivate-advice-list()
  viper-go-away()
  toggle-viper-mode()
  #[nil "\301 \210\30!\207" [before-buffer toggle-viper-mode switch-
  viper-test-undo-kmacro([105 49 32 50 32 51 32 52 32 53 32 54 escape
  #[nil "\304\305\306\307!D\x18\x19\310\x1a\311^[\312\216\313\"\211\x12)\204\x1c\0\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test viper-test-undo-3 "Test for VI like
  ert-run-or-rerun-test([cl-struct-ert--stats (not (tag :expensive-tes
  ert-run-tests((not (tag :expensive-test)) #[385 "\306\x02\307\"\203G\0\2
  ert-run-tests-batch((not (tag :expensive-test)))
  ert-run-tests-batch-and-exit((not (tag :expensive-test)))
  eval((ert-run-tests-batch-and-exit (quote (not (tag :expensive-test)
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emulation/viper-test
  command-line()
  normal-top-level()
Test viper-test-undo-3 condition:
    (wrong-number-of-arguments
     (2 . 2)
     1)
   FAILED  5/6  viper-test-undo-3
undo!
undo more!
undo more!
Test viper-test-undo-4 backtrace:
  viper--deactivate-advice-list()
  viper-go-away()
  toggle-viper-mode()
  #[nil "\301 \210\30!\207" [before-buffer toggle-viper-mode switch-
  viper-test-undo-kmacro([105 49 escape 111 50 escape 111 51 escape 11
  #[nil "\304\305\306\307!D\x18\x19\310\x1a\311^[\312\216\313\"\211\x12)\204\x1c\0\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test viper-test-undo-4 nil #[nil "\304\3
  ert-run-or-rerun-test([cl-struct-ert--stats (not (tag :expensive-tes
  ert-run-tests((not (tag :expensive-test)) #[385 "\306\x02\307\"\203G\0\2
  ert-run-tests-batch((not (tag :expensive-test)))
  ert-run-tests-batch-and-exit((not (tag :expensive-test)))
  eval((ert-run-tests-batch-and-exit (quote (not (tag :expensive-test)
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emulation/viper-test
  command-line()
  normal-top-level()
Test viper-test-undo-4 condition:
    (wrong-number-of-arguments
     (2 . 2)
     1)
   FAILED  6/6  viper-test-undo-4

Ran 6 tests, 1 results as expected, 5 unexpected (2016-05-26 13:26:25-0700)

5 unexpected results:
   FAILED  viper-test-fix
   FAILED  viper-test-undo-1
   FAILED  viper-test-undo-2
   FAILED  viper-test-undo-3
   FAILED  viper-test-undo-4

ERROR: lisp/emulation/viper-tests.log

bug#23625: viper-tests.el fails in master

[Phillip Lord]

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


Paul Eggert <eggert@cs.ucla.edu> writes:

> Phillip, I just now merged emacs-25 into master and fixed all the merge
> conflicts that were easy, but one problem remains: viper-tests, which you
> recently introduced to emacs-25, fails in the new master. Could you please
> look into this? I don't offhand know what viper-tests is supposed to do and
> don't know how to interpret the test failures. Thanks.

They are "do this, do that" keyboard macros. Hard to explain what they
do other than "this is how viper is supposed to behave".

Anyway, I think that the tests are working -- that is, they are picking
up a real bug, caused independently by 088acab3 which was from Stefan.
I've attached a fix. Stefan, can you check I have this right?

> (Is there any way to fix the test so that it outputs ordinary text when it
> fails?)

>   #[nil "\301 \210\30!\207" [before-buffer toggle-viper-mode switch-

You mean this bit? To be honest I am not sure where that bit of trace is
coming from.

Phil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-broken-viper-deactivation.patch --]
[-- Type: text/x-diff, Size: 1123 bytes --]

From 76409ca9daf72b697db9c1983aa322a365bfc066 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Thu, 26 May 2016 22:18:32 +0100
Subject: [PATCH] Fix broken viper deactivation

* lisp/emulation/viper.el (viper--deactivate-advice-list): Destructure
  args to advice-remove.
  (viper--advice-add): Use cons not list.
Addresses bug#23625
---
 lisp/emulation/viper.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/emulation/viper.el b/lisp/emulation/viper.el
index a0bba9a..1ee1464 100644
--- a/lisp/emulation/viper.el
+++ b/lisp/emulation/viper.el
@@ -647,10 +647,14 @@ viper--advice-list
 
 (defun viper--advice-add (function where advice)
   (advice-add function where advice)
-  (push (list function advice) viper--advice-list))
+  (push (cons function advice) viper--advice-list))
 
 (defun viper--deactivate-advice-list ()
-  (mapc #'advice-remove viper--advice-list)
+  (mapc (lambda (n)
+          (advice-remove
+           (car n)
+           (cdr n)))
+        viper--advice-list)
   (setq viper--advice-list nil))
 
 (defun viper-go-away ()
-- 
2.8.3

bug#23625: viper-tests.el fails in master

[Andy Moreton]

On Thu 26 May 2016, Paul Eggert wrote:

> Phillip, I just now merged emacs-25 into master and fixed all the merge
> conflicts that were easy, but one problem remains: viper-tests, which you
> recently introduced to emacs-25, fails in the new master. Could you please
> look into this? I don't offhand know what viper-tests is supposed to do and
> don't know how to interpret the test failures. Thanks.
>
> The symptoms with Emacs master commit 9a834970e822db10bc887db343fd8561a3639a66
> compiled for Fedora 23 x86-64, are that 'make check' fails with the attached
> output seemingly being the relevant part of the output. This attachment
> contains some control characters; I hope they survive the email process.
>
> (Is there any way to fix the test so that it outputs ordinary text when it
> fails?)

The control characters are from backtraces in the log - I assuem that
this is the bytecode representation from compiled elisp files.

    AndyM

bug#23625: viper-tests.el fails in master

[Stefan Monnier]

> Anyway, I think that the tests are working -- that is, they are picking
> up a real bug, caused independently by 088acab3 which was from Stefan.
> I've attached a fix. Stefan, can you check I have this right?

Indeed, thanks, the patch looks fine.  The use of `list` instead of
`cons` was because I intended to do

    (mapc (apply-partially #'apply #'advice-remove) viper--advice-list)

but your code is at least as good.


        Stefan