bug#24402: 25.1.50; testcover-start breaks should-error

bug#24402: 25.1.50; testcover-start breaks should-error

[Gemini Lasswell]

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

Enabling code coverage on a file containing an ert test that uses should-error causes the test to fail.

Steps to reproduce:
1. Start emacs (I used: open nextstep/Emacs.app —args -Q)

2. Create a buffer containing the following code and save it as bug.el:
(require 'ert)
(require 'testcover)

(ert-deftest should-error-bug ()
  (should-error (my-error)))

(defun my-error ()
  (/ 1 0))

3. M-x eval-buffer RET

4. M-x ert RET t RET

5. The test passes.

6. M-x testcover-start RET bug.el RET

7. M-x ert RET t RET

8. The test fails. TAB TAB d produced the attached debugger output.


[-- Attachment #2: bug.txt --]
[-- Type: text/plain, Size: 17362 bytes --]

Debugger entered--Lisp error: (arith-error)
  apply(debug (error (arith-error)))
  ert--run-test-debugger([cl-struct-ert--test-execution-info [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil (testcover-after 3 (let ... ...)))))) nil :passed nil] [cl-struct-ert-test-failed nil nil (arith-error) ((t (lambda nil (testcover-1value 1 (/ 1 0)))) (t testcover-enter my-error (lambda nil (testcover-1value 1 (/ 1 0)))) (t my-error) (nil list 2 (my-error)) (nil let ((fn-7 (function testcover-after)) (args-8 (list 2 (my-error)))) (let ((value-9 (quote ert-form-evaluation-aborted-10))) (let (form-description-11) (let (... ...) (condition-case -condition- ... ...) (if errorp12 nil ...))) value-9)) (nil testcover-after 3 (let ((fn-7 (function testcover-after)) (args-8 (list 2 ...))) (let ((value-9 ...)) (let (form-description-11) (let ... ... ...)) value-9))) (t (lambda nil (testcover-after 3 (let (... ...) (let ... ... value-9))))) (t testcover-enter test@should-error-bug (lambda nil (testcover-after 3 (let (... ...) (let ... ... value-9))))) (t (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil ...))))) (t ert--run-test-internal #0) (t ert-run-test [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function ...))) nil :passed nil]) (t ert-run-or-rerun-test [cl-struct-ert--stats t [[cl-struct-ert-test should-error-bug nil (lambda nil ...) nil :passed nil]] #s(hash-table size 1 test eql rehash-size 1.5 rehash-threshold 0.8 data (should-error-bug 0)) [nil] [(22483 23504 1016 0)] [(22483 22641 595275 0)] 0 0 0 0 0 (22483 22641 578681 0) (22483 22641 595513 0) nil [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter ... ...)) nil :passed nil] 1473469392.100925] [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function ...))) nil :passed nil] #[385 "\306\x02\307\"\203)\0\211\211G\310U\203\x14\0\211@\202^[\0\311\312\313\x03GD\"\301\314\x02\302\242\300#\240\210\315\301\242!\207\306\x02\316\"\203\222\0\211\211G\317U\203A\0\211\x01A\262\x02\242\202H\0\311\312\313\x03GD\"\x01@\303\320\x02\204T\0\321\202U\0\322\323\x05!\324\x06\x06!\325\x06\a!\211\326U\203j\0\321\202n\0\327\330\x02\"\262\x01\331\x06\b!\211\326U\203~\0\321\202\202\0\327\332\x02\"\262\x01&\x06\210\333r\301\242q\210\f)\x03\"\207\306\x02\334\"\203\372\0\211\211G\317U\203\252\0\211\x01A\262\x02\242\202\261\0\311\312\313\x03GD\"\x01@r\301\242q\210\f\335\x03\x03\"\336\x02\x02\"\211\204\311\0\337\340!\210\211\317H\211\326H\r>\204\333\0\311\341\342\x03D\"\210\211\211\310\x06\aI\266\x03\x0e-\x02\343\313\344\"I\210\345\x03\x06\x06\"\210\346\x03\x02\"\266\203)\207\306\x02\347\"\203\204\x01\211\211G\350U\203\x12\x01\211\x01A\262\x02\242\202\x19\x01\311\312\313\x03GD\"\x01\211A\262\x03\242\x02@r\301\242q\210\f\335\x04\x04\"\336\x02\x02\"\211\317H\211\326H\r>\204A\x01\311\341\342\x03D\"\210\211\317H\262\x01\203g\x01\211\317H\211\326H\r>\204[\x01\311\341\342\x03D\"\210\211\211\317\351\x06	\x06	\"I\266\x03\x0e-\x02\343\x06\x06\351\x06	\x06	\"\"I\210\345\x03\x06\a\"\210\346\x03\x02\"\266\203)\207\352\353\x03\354#\205\215\x01\313\207" [nil (#<buffer *ert*>) (#4) message ert--results-ewoc cl-struct-ert--ewoc-entry-tags eql run-started 1 signal wrong-number-of-arguments nil ert--setup-results-buffer pop-to-buffer run-ended 2 "%sRan %s tests, %s results were as expected%s%s" "" "Aborted: " ert-stats-total ert-stats-completed-expected ert-stats-completed-unexpected 0 format ", %s unexpected" ert-stats-skipped ", %s skipped" ert--results-update-stats-display test-started ert--stats-test-pos ewoc-nth cl--assertion-failed node wrong-type-argument ert--ewoc-entry ert-char-for-test-result t ert--results-update-stats-display-maybe ewoc-invalidate test-ended 3 ert-test-result-expected-p error "cl-ecase failed: %s, %s" (run-started run-ended test-started test-ended) ert--results-progress-bar-string] 16 "\n\n(fn EVENT-TYPE &rest EVENT-ARGS)"]) (t ert-results-rerun-test-at-point) (t ert-results-rerun-test-at-point-debugging-errors) (t funcall-interactively ert-results-rerun-test-at-point-debugging-errors) (t call-interactively ert-results-rerun-test-at-point-debugging-errors nil nil) (t command-execute ert-results-rerun-test-at-point-debugging-errors)) nil] #[0 "\300\301\302\"\207" [throw --cl-block-error-- nil] 3] debug t] (error (arith-error)))
  #[128 "\301\300\x02\"\207" [[cl-struct-ert--test-execution-info [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil ...)))) nil :passed nil] [cl-struct-ert-test-failed nil nil (arith-error) ((t (lambda nil (testcover-1value 1 ...))) (t testcover-enter my-error (lambda nil (testcover-1value 1 ...))) (t my-error) (nil list 2 (my-error)) (nil let ((fn-7 ...) (args-8 ...)) (let (...) (let ... ...) value-9)) (nil testcover-after 3 (let (... ...) (let ... ... value-9))) (t (lambda nil (testcover-after 3 ...))) (t testcover-enter test@should-error-bug (lambda nil (testcover-after 3 ...))) (t (lambda nil (testcover-enter ... ...))) (t ert--run-test-internal #2) (t ert-run-test [cl-struct-ert-test should-error-bug nil (lambda nil ...) nil :passed nil]) (t ert-run-or-rerun-test [cl-struct-ert--stats t [[cl-struct-ert-test should-error-bug nil ... nil :passed nil]] #s(hash-table size 1 test eql rehash-size 1.5 rehash-threshold 0.8 data (should-error-bug 0)) [nil] [...] [...] 0 0 0 0 0 (22483 22641 578681 0) (22483 22641 595513 0) nil [cl-struct-ert-test should-error-bug nil ... nil :passed nil] 1473469392.100925] [cl-struct-ert-test should-error-bug nil (lambda nil ...) nil :passed nil] #[385 "\306\x02\307\"\203)\0\211\211G\310U\203\x14\0\211@\202^[\0\311\312\313\x03GD\"\301\314\x02\302\242\300#\240\210\315\301\242!\207\306\x02\316\"\203\222\0\211\211G\317U\203A\0\211\x01A\262\x02\242\202H\0\311\312\313\x03GD\"\x01@\303\320\x02\204T\0\321\202U\0\322\323\x05!\324\x06\x06!\325\x06\a!\211\326U\203j\0\321\202n\0\327\330\x02\"\262\x01\331\x06\b!\211\326U\203~\0\321\202\202\0\327\332\x02\"\262\x01&\x06\210\333r\301\242q\210\f)\x03\"\207\306\x02\334\"\203\372\0\211\211G\317U\203\252\0\211\x01A\262\x02\242\202\261\0\311\312\313\x03GD\"\x01@r\301\242q\210\f\335\x03\x03\"\336\x02\x02\"\211\204\311\0\337\340!\210\211\317H\211\326H\r>\204\333\0\311\341\342\x03D\"\210\211\211\310\x06\aI\266\x03\x0e-\x02\343\313\344\"I\210\345\x03\x06\x06\"\210\346\x03\x02\"\266\203)\207\306\x02\347\"\203\204\x01\211\211G\350U\203\x12\x01\211\x01A\262\x02\242\202\x19\x01\311\312\313\x03GD\"\x01\211A\262\x03\242\x02@r\301\242q\210\f\335\x04\x04\"\336\x02\x02\"\211\317H\211\326H\r>\204A\x01\311\341\342\x03D\"\210\211\317H\262\x01\203g\x01\211\317H\211\326H\r>\204[\x01\311\341\342\x03D\"\210\211\211\317\351\x06	\x06	\"I\266\x03\x0e-\x02\343\x06\x06\351\x06	\x06	\"\"I\210\345\x03\x06\a\"\210\346\x03\x02\"\266\203)\207\352\353\x03\354#\205\215\x01\313\207" [nil ... ... message ert--results-ewoc cl-struct-ert--ewoc-entry-tags eql run-started 1 signal wrong-number-of-arguments nil ert--setup-results-buffer pop-to-buffer run-ended 2 "%sRan %s tests, %s results were as expected%s%s" "" "Aborted: " ert-stats-total ert-stats-completed-expected ert-stats-completed-unexpected 0 format ", %s unexpected" ert-stats-skipped ", %s skipped" ert--results-update-stats-display test-started ert--stats-test-pos ewoc-nth cl--assertion-failed node wrong-type-argument ert--ewoc-entry ert-char-for-test-result t ert--results-update-stats-display-maybe ewoc-invalidate test-ended 3 ert-test-result-expected-p error "cl-ecase failed: %s, %s" ... ert--results-progress-bar-string] 16 "\n\n(fn EVENT-TYPE &rest EVENT-ARGS)"]) (t ert-results-rerun-test-at-point) (t ert-results-rerun-test-at-point-debugging-errors) (t funcall-interactively ert-results-rerun-test-at-point-debugging-errors) (t call-interactively ert-results-rerun-test-at-point-debugging-errors nil nil) (t command-execute ert-results-rerun-test-at-point-debugging-errors)) nil] #[0 "\300\301\302\"\207" [throw --cl-block-error-- nil] 3] debug t] ert--run-test-debugger] 4 "\n\n(fn &rest ARGS)"](error (arith-error))
  /(1 0)
  (testcover-1value 1 (/ 1 0))
  (lambda nil (testcover-1value 1 (/ 1 0)))()
  testcover-enter(my-error (lambda nil (testcover-1value 1 (/ 1 0))))
  my-error()
  (list 2 (my-error))
  (let ((fn-7 (function testcover-after)) (args-8 (list 2 (my-error)))) (let ((value-9 (quote ert-form-evaluation-aborted-10))) (let (form-description-11) (let ((errorp12 nil) (form-description-fn-13 (function (lambda nil form-description-11)))) (condition-case -condition- (unwind-protect (setq value-9 (apply fn-7 args-8)) (setq form-description-11 (nconc ... ... ... ...)) (ert--signal-should-execution form-description-11)) (error (setq errorp12 t) (ert--should-error-handle-error form-description-fn-13 -condition- (quote error) nil) (setq value-9 -condition-))) (if errorp12 nil (ert-fail (append (funcall form-description-fn-13) (list :fail-reason "did not signal an error")))))) value-9))
  (testcover-after 3 (let ((fn-7 (function testcover-after)) (args-8 (list 2 (my-error)))) (let ((value-9 (quote ert-form-evaluation-aborted-10))) (let (form-description-11) (let ((errorp12 nil) (form-description-fn-13 (function ...))) (condition-case -condition- (unwind-protect (setq value-9 ...) (setq form-description-11 ...) (ert--signal-should-execution form-description-11)) (error (setq errorp12 t) (ert--should-error-handle-error form-description-fn-13 -condition- ... nil) (setq value-9 -condition-))) (if errorp12 nil (ert-fail (append ... ...))))) value-9)))
  (lambda nil (testcover-after 3 (let ((fn-7 (function testcover-after)) (args-8 (list 2 (my-error)))) (let ((value-9 (quote ert-form-evaluation-aborted-10))) (let (form-description-11) (let ((errorp12 nil) (form-description-fn-13 ...)) (condition-case -condition- (unwind-protect ... ... ...) (error ... ... ...)) (if errorp12 nil (ert-fail ...)))) value-9))))()
  testcover-enter(test@should-error-bug (lambda nil (testcover-after 3 (let ((fn-7 (function testcover-after)) (args-8 (list 2 (my-error)))) (let ((value-9 (quote ert-form-evaluation-aborted-10))) (let (form-description-11) (let ((errorp12 nil) (form-description-fn-13 ...)) (condition-case -condition- (unwind-protect ... ... ...) (error ... ... ...)) (if errorp12 nil (ert-fail ...)))) value-9)))))
  (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil (testcover-after 3 (let ((fn-7 ...) (args-8 ...)) (let (...) (let ... ...) value-9)))))))()
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil (testcover-after 3 (let ... ...)))))) nil :passed nil] [cl-struct-ert-test-failed nil nil (arith-error) ((t (lambda nil (testcover-1value 1 (/ 1 0)))) (t testcover-enter my-error (lambda nil (testcover-1value 1 (/ 1 0)))) (t my-error) (nil list 2 (my-error)) (nil let ((fn-7 (function testcover-after)) (args-8 (list 2 (my-error)))) (let ((value-9 (quote ert-form-evaluation-aborted-10))) (let (form-description-11) (let (... ...) (condition-case -condition- ... ...) (if errorp12 nil ...))) value-9)) (nil testcover-after 3 (let ((fn-7 (function testcover-after)) (args-8 (list 2 ...))) (let ((value-9 ...)) (let (form-description-11) (let ... ... ...)) value-9))) (t (lambda nil (testcover-after 3 (let (... ...) (let ... ... value-9))))) (t testcover-enter test@should-error-bug (lambda nil (testcover-after 3 (let (... ...) (let ... ... value-9))))) (t (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil ...))))) (t ert--run-test-internal #0) (t ert-run-test [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function ...))) nil :passed nil]) (t ert-run-or-rerun-test [cl-struct-ert--stats t [[cl-struct-ert-test should-error-bug nil (lambda nil ...) nil :passed nil]] #s(hash-table size 1 test eql rehash-size 1.5 rehash-threshold 0.8 data (should-error-bug 0)) [nil] [(22483 23504 1016 0)] [(22483 22641 595275 0)] 0 0 0 0 0 (22483 22641 578681 0) (22483 22641 595513 0) nil [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter ... ...)) nil :passed nil] 1473469392.100925] [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function ...))) nil :passed nil] #[385 "\306\x02\307\"\203)\0\211\211G\310U\203\x14\0\211@\202^[\0\311\312\313\x03GD\"\301\314\x02\302\242\300#\240\210\315\301\242!\207\306\x02\316\"\203\222\0\211\211G\317U\203A\0\211\x01A\262\x02\242\202H\0\311\312\313\x03GD\"\x01@\303\320\x02\204T\0\321\202U\0\322\323\x05!\324\x06\x06!\325\x06\a!\211\326U\203j\0\321\202n\0\327\330\x02\"\262\x01\331\x06\b!\211\326U\203~\0\321\202\202\0\327\332\x02\"\262\x01&\x06\210\333r\301\242q\210\f)\x03\"\207\306\x02\334\"\203\372\0\211\211G\317U\203\252\0\211\x01A\262\x02\242\202\261\0\311\312\313\x03GD\"\x01@r\301\242q\210\f\335\x03\x03\"\336\x02\x02\"\211\204\311\0\337\340!\210\211\317H\211\326H\r>\204\333\0\311\341\342\x03D\"\210\211\211\310\x06\aI\266\x03\x0e-\x02\343\313\344\"I\210\345\x03\x06\x06\"\210\346\x03\x02\"\266\203)\207\306\x02\347\"\203\204\x01\211\211G\350U\203\x12\x01\211\x01A\262\x02\242\202\x19\x01\311\312\313\x03GD\"\x01\211A\262\x03\242\x02@r\301\242q\210\f\335\x04\x04\"\336\x02\x02\"\211\317H\211\326H\r>\204A\x01\311\341\342\x03D\"\210\211\317H\262\x01\203g\x01\211\317H\211\326H\r>\204[\x01\311\341\342\x03D\"\210\211\211\317\351\x06	\x06	\"I\266\x03\x0e-\x02\343\x06\x06\351\x06	\x06	\"\"I\210\345\x03\x06\a\"\210\346\x03\x02\"\266\203)\207\352\353\x03\354#\205\215\x01\313\207" [nil (#<buffer *ert*>) (#4) message ert--results-ewoc cl-struct-ert--ewoc-entry-tags eql run-started 1 signal wrong-number-of-arguments nil ert--setup-results-buffer pop-to-buffer run-ended 2 "%sRan %s tests, %s results were as expected%s%s" "" "Aborted: " ert-stats-total ert-stats-completed-expected ert-stats-completed-unexpected 0 format ", %s unexpected" ert-stats-skipped ", %s skipped" ert--results-update-stats-display test-started ert--stats-test-pos ewoc-nth cl--assertion-failed node wrong-type-argument ert--ewoc-entry ert-char-for-test-result t ert--results-update-stats-display-maybe ewoc-invalidate test-ended 3 ert-test-result-expected-p error "cl-ecase failed: %s, %s" (run-started run-ended test-started test-ended) ert--results-progress-bar-string] 16 "\n\n(fn EVENT-TYPE &rest EVENT-ARGS)"]) (t ert-results-rerun-test-at-point) (t ert-results-rerun-test-at-point-debugging-errors) (t funcall-interactively ert-results-rerun-test-at-point-debugging-errors) (t call-interactively ert-results-rerun-test-at-point-debugging-errors nil nil) (t command-execute ert-results-rerun-test-at-point-debugging-errors)) nil] #[0 "\300\301\302\"\207" [throw --cl-block-error-- nil] 3] debug t])
  ert-run-test([cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil (testcover-after 3 (let (... ...) (let ... ... value-9))))))) nil :passed nil])
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil (testcover-after 3 ...))))) nil :passed nil]] #s(hash-table size 1 test eql rehash-size 1.5 rehash-threshold 0.8 data (should-error-bug 0)) [nil] [(22483 23504 1016 0)] [(22483 22641 595275 0)] 0 0 0 0 0 (22483 22641 578681 0) (22483 22641 595513 0) nil [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil (testcover-after 3 (let ... ...)))))) nil :passed nil] 1473469392.100925] [cl-struct-ert-test should-error-bug nil (lambda nil (testcover-enter (quote test@should-error-bug) (function (lambda nil (testcover-after 3 (let (... ...) (let ... ... value-9))))))) nil :passed nil] #[385 "\306\x02\307\"\203)\0\211\211G\310U\203\x14\0\211@\202^[\0\311\312\313\x03GD\"\301\314\x02\302\242\300#\240\210\315\301\242!\207\306\x02\316\"\203\222\0\211\211G\317U\203A\0\211\x01A\262\x02\242\202H\0\311\312\313\x03GD\"\x01@\303\320\x02\204T\0\321\202U\0\322\323\x05!\324\x06\x06!\325\x06\a!\211\326U\203j\0\321\202n\0\327\330\x02\"\262\x01\331\x06\b!\211\326U\203~\0\321\202\202\0\327\332\x02\"\262\x01&\x06\210\333r\301\242q\210\f)\x03\"\207\306\x02\334\"\203\372\0\211\211G\317U\203\252\0\211\x01A\262\x02\242\202\261\0\311\312\313\x03GD\"\x01@r\301\242q\210\f\335\x03\x03\"\336\x02\x02\"\211\204\311\0\337\340!\210\211\317H\211\326H\r>\204\333\0\311\341\342\x03D\"\210\211\211\310\x06\aI\266\x03\x0e-\x02\343\313\344\"I\210\345\x03\x06\x06\"\210\346\x03\x02\"\266\203)\207\306\x02\347\"\203\204\x01\211\211G\350U\203\x12\x01\211\x01A\262\x02\242\202\x19\x01\311\312\313\x03GD\"\x01\211A\262\x03\242\x02@r\301\242q\210\f\335\x04\x04\"\336\x02\x02\"\211\317H\211\326H\r>\204A\x01\311\341\342\x03D\"\210\211\317H\262\x01\203g\x01\211\317H\211\326H\r>\204[\x01\311\341\342\x03D\"\210\211\211\317\351\x06	\x06	\"I\266\x03\x0e-\x02\343\x06\x06\351\x06	\x06	\"\"I\210\345\x03\x06\a\"\210\346\x03\x02\"\266\203)\207\352\353\x03\354#\205\215\x01\313\207" [nil (#<buffer *ert*>) (#0) message ert--results-ewoc cl-struct-ert--ewoc-entry-tags eql run-started 1 signal wrong-number-of-arguments nil ert--setup-results-buffer pop-to-buffer run-ended 2 "%sRan %s tests, %s results were as expected%s%s" "" "Aborted: " ert-stats-total ert-stats-completed-expected ert-stats-completed-unexpected 0 format ", %s unexpected" ert-stats-skipped ", %s skipped" ert--results-update-stats-display test-started ert--stats-test-pos ewoc-nth cl--assertion-failed node wrong-type-argument ert--ewoc-entry ert-char-for-test-result t ert--results-update-stats-display-maybe ewoc-invalidate test-ended 3 ert-test-result-expected-p error "cl-ecase failed: %s, %s" (run-started run-ended test-started test-ended) ert--results-progress-bar-string] 16 "\n\n(fn EVENT-TYPE &rest EVENT-ARGS)"])
  ert-results-rerun-test-at-point()
  ert-results-rerun-test-at-point-debugging-errors()
  funcall-interactively(ert-results-rerun-test-at-point-debugging-errors)
  call-interactively(ert-results-rerun-test-at-point-debugging-errors nil nil)
  command-execute(ert-results-rerun-test-at-point-debugging-errors)

[-- Attachment #3: Type: text/plain, Size: 2705 bytes --]



In GNU Emacs 25.1.50.8 (x86_64-apple-darwin15.6.0, NS appkit-1404.47 Version 10.11.6 (Build 15G1004))
 of 2016-09-09 built on rainbow.local
Windowing system distributor 'Apple', version 10.3.1404
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
You can run the command ‘eval-buffer’ with M-x ev-b RET
Ran 1 tests, 1 results were as expected
Edebug: test@should-error-bug
Edebug: my-error
You can run the command ‘testcover-start’ with M-x tes-s RET
Edebug: my-error
Ran 1 tests, 0 results were as expected, 1 unexpected

Configured using:
 'configure --with-ns --without-gnutls'

Configured features:
JPEG IMAGEMAGICK NOTIFY ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ERT-Results

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny dired dired-loaddefs
format-spec rfc822 mml mml-sec password-cache epa derived epg epg-config
gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils cl-seq cl-macs testcover edebug
ert pp find-func seq byte-opt gv bytecomp byte-compile cl-extra
help-mode cconv ewoc easymenu debug cl-loaddefs pcase cl-lib time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/ns-win ns-win ucs-normalize term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list newcomment
elisp-mode lisp-mode prog-mode register page menu-bar rfn-eshadow timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese charscript case-table epa-hook jka-cmpr-hook help simple abbrev
obarray minibuffer cl-preloaded nadvice loaddefs button faces cus-face
macroexp files text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable backquote kqueue
cocoa ns multi-tty make-network-process emacs)

bug#24402: More Information

[Gemini Lasswell]

I’ve done some investigating of why this is happening. testcover-start
transforms:
    (should-error (my-error))
into:
    (should-error (testcover-after 2 (my-error)))

Then the macro expansion of should-error separates the form which it
is passed into a function symbol and list of arguments, and applies
the function to the arguments inside of a condition-case that traps
errors. The problem is that the arguments are evaluated first, outside
of the condition-case, so errors in their evaluation do not get
caught. This problem is not specific to testcover. In this example,
the first test passes and the second fails:

(defun div-by (n)
  (/ 100 n))

(defmacro log-div-by (n)
  `(message "div-by: %d" (div-by ,n)))

(ert-deftest test-div-by ()
  (should-error (div-by 0)))

(ert-deftest test-log-div-by ()
  (should-error (log-div-by 0)))
  

bug#24402: should-error doesn't catch all errors (Was:bug#24402: More Information)

[Alex]

tags + 24402 confirmed
quit

Gemini Lasswell <gazally@runbox.com> writes:

> I’ve done some investigating of why this is happening. testcover-start
> transforms:
>     (should-error (my-error))
> into:
>     (should-error (testcover-after 2 (my-error)))
>
> Then the macro expansion of should-error separates the form which it
> is passed into a function symbol and list of arguments, and applies
> the function to the arguments inside of a condition-case that traps
> errors. The problem is that the arguments are evaluated first, outside
> of the condition-case, so errors in their evaluation do not get
> caught. This problem is not specific to testcover. In this example,
> the first test passes and the second fails:
>
> (defun div-by (n)
>   (/ 100 n))
>
> (defmacro log-div-by (n)
>   `(message "div-by: %d" (div-by ,n)))
>
> (ert-deftest test-div-by ()
>   (should-error (div-by 0)))
>
> (ert-deftest test-log-div-by ()
>   (should-error (log-div-by 0)))
>   

I just ran into this as well. Consider these two forms:

(should-error (cl-fourth "1234") :type 'wrong-type-argument)

(should-error (car (cdr (cdr (cdr "1234")))) :type 'wrong-type-argument)

Only the second raises an error, even though cl-fourth is equivalent to
the car/cdr chain.

bug#24402: should-error doesn't catch all errors

[Alex]

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

tags 24402 patch
quit

Alex <agrambot@gmail.com> writes:

> Gemini Lasswell <gazally@runbox.com> writes:
>
>> I’ve done some investigating of why this is happening. testcover-start
>> transforms:
>>     (should-error (my-error))
>> into:
>>     (should-error (testcover-after 2 (my-error)))
>>
>> Then the macro expansion of should-error separates the form which it
>> is passed into a function symbol and list of arguments, and applies
>> the function to the arguments inside of a condition-case that traps
>> errors. The problem is that the arguments are evaluated first, outside
>> of the condition-case, so errors in their evaluation do not get
>> caught. This problem is not specific to testcover. In this example,
>> the first test passes and the second fails:
>>
>> (defun div-by (n)
>>   (/ 100 n))
>>
>> (defmacro log-div-by (n)
>>   `(message "div-by: %d" (div-by ,n)))
>>
>> (ert-deftest test-div-by ()
>>   (should-error (div-by 0)))
>>
>> (ert-deftest test-log-div-by ()
>>   (should-error (log-div-by 0)))
>>   
>
> I just ran into this as well. Consider these two forms:
>
> (should-error (cl-fourth "1234") :type 'wrong-type-argument)
>
> (should-error (car (cdr (cdr (cdr "1234")))) :type 'wrong-type-argument)
>
> Only the second raises an error, even though cl-fourth is equivalent to
> the car/cdr chain.

Here's a diff that appears to fix this. It's a bit crude, but I'd rather
not mess around too much with ert's internals.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: first --]
[-- Type: text/x-diff, Size: 918 bytes --]

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index 2c49a634e3..f5f3e30c0d 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -303,8 +303,12 @@ ert--expand-should-1
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (list ,@arg-forms)
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))

[-- Attachment #3: Type: text/plain, Size: 254 bytes --]


There's a similar issue with macro-expansions inside of
should/should-error/should-not that could/should be fixed by wrapping
the macroexpand call at the top of ert--expand-should-1 in a similar
condition-case. Here's another diff that does just that:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: second --]
[-- Type: text/x-diff, Size: 2316 bytes --]

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..66ae31fd51 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -276,13 +276,15 @@ ert--special-operator-p
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         (condition-case err
+             (macroexpand-all form (append (bound-and-true-p
+                                            byte-compile-macro-environment)
+                                           (cond
+                                            ((boundp 'macroexpand-all-environment)
+                                             macroexpand-all-environment)
+                                            ((boundp 'cl-macro-environment)
+                                             cl-macro-environment))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -303,8 +305,13 @@ ert--expand-should-1
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (list ,@arg-forms)
+                           ;; (ert-test-failed (signal (car err) (cdr err)))
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))

[-- Attachment #5: Type: text/plain, Size: 622 bytes --]


I ran "make check" and found only one test that the above diff breaks:
ert-test-test-result-expected-p.

I can't seem to figure out why it doesn't work. The test fails because
of these two:

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (should-not (ert-test-result-expected-p test (ert-run-test test))))
(let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
                           :expected-result-type ':failed)))
  (should (ert-test-result-expected-p test (ert-run-test test))))

I tried to re-throw the ert-test-failed signal and still the above two
forms raise error an error.

bug#24402: should-error doesn't catch all errors

[Tino Calancha]

Alex <agrambot@gmail.com> writes:

> I ran "make check" and found only one test that the above diff breaks:
> ert-test-test-result-expected-p.
>
> I can't seem to figure out why it doesn't work. The test fails because
> of these two:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (should-not (ert-test-result-expected-p test (ert-run-test test))))
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
>                            :expected-result-type ':failed)))
>   (should (ert-test-result-expected-p test (ert-run-test test))))
>
> I tried to re-throw the ert-test-failed signal and still the above two
> forms raise error an error.

Hi!
I just arrived from teletransportation from Bug#27559.  Very fast! (and
cheap!).

Thank you for looking on this.  I think you go in the right direction to
fix this problem.

* I have updated your patch and all the test suite pass (even your
  proposed tests in Bug#27559 without requiring "(eval '....)").

* Bear in mind that I am far to be an expert on `ert.el', and i am
  already in my second beer, so please double check that
  the patch have sense.
  
--8<-----------------------------cut here---------------start------------->8---
commit a07f99f062f3da3418060ef30e1a00030fa0b947
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Wed Jul 5 22:11:46 2017 +0900

    Tweak Alex's 2nd patch

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..2d131cf99e 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -276,13 +276,15 @@ ert--special-operator-p
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         (condition-case err
+             (macroexpand-all form (append (bound-and-true-p
+                                            byte-compile-macro-environment)
+                                           (cond
+                                            ((boundp 'macroexpand-all-environment)
+                                             macroexpand-all-environment)
+                                            ((boundp 'cl-macro-environment)
+                                             cl-macro-environment))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -303,8 +305,14 @@ ert--expand-should-1
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (list ,@arg-forms)
+                           (error (if (or (eq (car err) 'ert-test-failed)
+                                          (eq (car err) 'ert-test-skipped))
+                                      (list ,@arg-forms)
+                                    (setq ,fn #'signal)
+                                    (list (car err) (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-07-05
Repository revision: 5d62247323f53f3ae9c7d9f51e951635887b2fb6

bug#24402: should-error doesn't catch all errors

[Alex]

Tino Calancha <tino.calancha@gmail.com> writes:

> Hi!
> I just arrived from teletransportation from Bug#27559.  Very fast! (and
> cheap!).
>
> Thank you for looking on this.  I think you go in the right direction to
> fix this problem.
>
> * I have updated your patch and all the test suite pass (even your
>   proposed tests in Bug#27559 without requiring "(eval '....)").
>
> * Bear in mind that I am far to be an expert on `ert.el', and i am
>   already in my second beer, so please double check that
>   the patch have sense.
>   
>
> commit a07f99f062f3da3418060ef30e1a00030fa0b947
> Author: Tino Calancha <tino.calancha@gmail.com>
> Date:   Wed Jul 5 22:11:46 2017 +0900
>
>     Tweak Alex's 2nd patch
>
> diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
> index eb2b2e3e11..2d131cf99e 100644
> --- a/lisp/emacs-lisp/ert.el
> +++ b/lisp/emacs-lisp/ert.el
> @@ -276,13 +276,15 @@ ert--special-operator-p
>  (defun ert--expand-should-1 (whole form inner-expander)
>    "Helper function for the `should' macro and its variants."
>    (let ((form
> -         (macroexpand form (append (bound-and-true-p
> -                                    byte-compile-macro-environment)
> -                                   (cond
> -                                    ((boundp 'macroexpand-all-environment)
> -                                     macroexpand-all-environment)
> -                                    ((boundp 'cl-macro-environment)
> -                                     cl-macro-environment))))))
> +         (condition-case err
> +             (macroexpand-all form (append (bound-and-true-p
> +                                            byte-compile-macro-environment)
> +                                           (cond
> +                                            ((boundp 'macroexpand-all-environment)
> +                                             macroexpand-all-environment)
> +                                            ((boundp 'cl-macro-environment)
> +                                             cl-macro-environment))))
> +           (error `(signal ',(car err) ',(cdr err))))))
>      (cond
>       ((or (atom form) (ert--special-operator-p (car form)))
>        (let ((value (cl-gensym "value-")))
> @@ -303,8 +305,14 @@ ert--expand-should-1
>                (args (cl-gensym "args-"))
>                (value (cl-gensym "value-"))
>                (default-value (cl-gensym "ert-form-evaluation-aborted-")))
> -          `(let ((,fn (function ,fn-name))
> -                 (,args (list ,@arg-forms)))
> +          `(let* ((,fn (function ,fn-name))
> +                  (,args (condition-case err
> +                             (list ,@arg-forms)
> +                           (error (if (or (eq (car err) 'ert-test-failed)
> +                                          (eq (car err) 'ert-test-skipped))
> +                                      (list ,@arg-forms)
> +                                    (setq ,fn #'signal)
> +                                    (list (car err) (cdr err)))))))
>               (let ((,value ',default-value))
>                 ,(funcall inner-expander
>                           `(setq ,value (apply ,fn ,args))

Thanks, that helps with my understanding of the issue. Sadly, I don't
think your tweak will work in all cases, namely whenever (list
,@arg-forms) has side-effects. Luckily this doesn't happen in any
current tests, but I think those types of tests should be supported.

I believe the following is why my previous diff doesn't work. Consider:

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (ert-run-test test))

The above returns a struct/record and does not error.

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (condition-case err
      (ert-run-test test)
    (error "woops")))

Even though ert-run-test by itself does not error, the error handler is
ran. I believe this is because `ert--run-test-internal' binds `debugger'
around the evaluation of the test to somehow suppress this error.

Using condition-case-unless-debug gets around this, but now anytime
debug-on-error is non-nil the condition-case won't catch the error. I'm
not sure how to solve this.

bug#24402: should-error doesn't catch all errors

[Tino Calancha]

Alex <agrambot@gmail.com> writes:

> I don't think your tweak will work in all cases, namely whenever
> (list  ,@arg-forms) has side-effects. Luckily this doesn't happen in any
> current tests, but I think those types of tests should be supported.
Good point.

> I believe the following is why my previous diff doesn't work. Consider:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (ert-run-test test))
>
> The above returns a struct/record and does not error.
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (condition-case err
>       (ert-run-test test)
>     (error "woops")))
>
> Even though ert-run-test by itself does not error, the error handler is
> ran. I believe this is because `ert--run-test-internal' binds `debugger'
> around the evaluation of the test to somehow suppress this error.
>
> Using condition-case-unless-debug gets around this, but now anytime
> debug-on-error is non-nil the condition-case won't catch the error. I'm
> not sure how to solve this.
I've being thinking about this, and i cannot find something better than
your second patch.

My suggestion is:

1. We apply your 2nd patch.
2. We ammend the failing tests wrapping '> I don't think your tweak will work in all cases, namely whenever
> (list  ,@arg-forms) has side-effects. Luckily this doesn't happen in any
> current tests, but I think those types of tests should be supported.
Good point.

> I believe the following is why my previous diff doesn't work. Consider:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (ert-run-test test))
>
> The above returns a struct/record and does not error.
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (condition-case err
>       (ert-run-test test)
>     (error "woops")))
>
> Even though ert-run-test by itself does not error, the error handler is
> ran. I believe this is because `ert--run-test-internal' binds `debugger'
> around the evaluation of the test to somehow suppress this error.
>
> Using condition-case-unless-debug gets around this, but now anytime
> debug-on-error is non-nil the condition-case won't catch the error. I'm
> not sure how to solve this.
I've being thinking about this, and i cannot find something better than
your second patch.

My suggestion is:

1. We apply your 2nd patch.
2. We handle the failing tests wrapping '(ert-fail "failed")' into
  `ignore-errors' as in below patch.

Then, IMO we are in a better situation than we are know:
That fix this bug, and if i am nt missing something, at a low price: just
tweaking a bit 2 innocents tests.

What do you think?


--8<-----------------------------cut here---------------start------------->8---

diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..f3e4db192a 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -413,12 +413,14 @@ ert-test--which-file
   (let ((test (make-ert-test :body (lambda ()))))
     (should (ert-test-result-expected-p test (ert-run-test test))))
   ;; unexpected failure
-  (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
-    (should-not (ert-test-result-expected-p test (ert-run-test test))))
-  ;; expected failure
-  (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
-                             :expected-result-type ':failed)))
+  (let ((test (make-ert-test
+               :body (lambda () (ignore-errors (ert-fail "failed"))))))
     (should (ert-test-result-expected-p test (ert-run-test test))))
+  ;; expected failure
+  (let ((test (make-ert-test
+               :body (lambda () (ignore-errors (ert-fail "failed")))
+               :expected-result-type ':failed)))
+    (should-not (ert-test-result-expected-p test (ert-run-test test))))
   ;; `not' expected type
   (let ((test (make-ert-test :body (lambda ())
                              :expected-result-type '(not :failed))))
--8<-----------------------------cut here---------------end--------------->8---

bug#24402: should-error doesn't catch all errors

[Alex]

Tino Calancha <tino.calancha@gmail.com> writes:

> I've being thinking about this, and i cannot find something better than
> your second patch.
>
> My suggestion is:
>
> 1. We apply your 2nd patch.
> 2. We handle the failing tests wrapping '(ert-fail "failed")' into
>   `ignore-errors' as in below patch.
>
> Then, IMO we are in a better situation than we are know:
> That fix this bug, and if i am nt missing something, at a low price: just
> tweaking a bit 2 innocents tests.
>
> What do you think?
>
>
> diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
> index 317838b250..f3e4db192a 100644
> --- a/test/lisp/emacs-lisp/ert-tests.el
> +++ b/test/lisp/emacs-lisp/ert-tests.el
> @@ -413,12 +413,14 @@ ert-test--which-file
>    (let ((test (make-ert-test :body (lambda ()))))
>      (should (ert-test-result-expected-p test (ert-run-test test))))
>    ;; unexpected failure
> -  (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
> -    (should-not (ert-test-result-expected-p test (ert-run-test test))))
> -  ;; expected failure
> -  (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
> -                             :expected-result-type ':failed)))
> +  (let ((test (make-ert-test
> +               :body (lambda () (ignore-errors (ert-fail "failed"))))))
>      (should (ert-test-result-expected-p test (ert-run-test test))))
> +  ;; expected failure
> +  (let ((test (make-ert-test
> +               :body (lambda () (ignore-errors (ert-fail "failed")))
> +               :expected-result-type ':failed)))
> +    (should-not (ert-test-result-expected-p test (ert-run-test test))))
>    ;; `not' expected type
>    (let ((test (make-ert-test :body (lambda ())
>                               :expected-result-type '(not :failed))))

I agree that it's a low price, and that it fixes more than it breaks,
but it does involve switching the logic in a simple basic test.

Also note that ignore-errors expands into a condition-case, so instead
of changing the tests, the patch could be tweaked to return nil if the
test signals ert-test-{failed, skipped}. Or the two tests could use
condition-case directly and make sure that ert-test-failed is/isn't
signalled.

I'd like to get some more opinions on this bug, in the hopes that
there's a better solution. Eli/Noam (or anyone happening upon this), do
you have any ideas on how to solve this?

bug#24402: should-error doesn't catch all errors

[npostavs]

Alex <agrambot@gmail.com> writes:

> I believe the following is why my previous diff doesn't work. Consider:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (ert-run-test test))
>
> The above returns a struct/record and does not error.
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (condition-case err
>       (ert-run-test test)
>     (error "woops")))
>
> Even though ert-run-test by itself does not error, the error handler is
> ran. I believe this is because `ert--run-test-internal' binds `debugger'
> around the evaluation of the test to somehow suppress this error.

Yes, ert binds `debugger' in order to get full backtrace information
when there is an error.  This means it won't see errors caught by
condition-case.  That's good when it ignores errors caught by test code
using condition-case, but does give rise to problems.  There is some
relevant discussion in Bugs #11218 and #24617.

Espcially the suggestion in #24617 of using `signal-hook-function' to
record error info instead of using `debugger', I think doing this could
simplify things a lot.  It is definitely going to require messing around
with ert's internals though...

bug#24402: should-error doesn't catch all errors

[Alex]

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

npostavs@users.sourceforge.net writes:

> Yes, ert binds `debugger' in order to get full backtrace information
> when there is an error.  This means it won't see errors caught by
> condition-case.  That's good when it ignores errors caught by test code
> using condition-case, but does give rise to problems.  There is some
> relevant discussion in Bugs #11218 and #24617.
>
> Espcially the suggestion in #24617 of using `signal-hook-function' to
> record error info instead of using `debugger', I think doing this could
> simplify things a lot.  It is definitely going to require messing around
> with ert's internals though...

Thanks for the info. I may have discovered a workaround, but I'm not
sure if there's any negative side-effects. All the tests pass, though.

What do you think of it? It's obviously not ideal, but I think it at
least fixes the issues at hand.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3 --]
[-- Type: text/x-diff, Size: 3815 bytes --]

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..beb32c48ce 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -266,6 +266,14 @@ ert--signal-should-execution
   (when ert--should-execution-observer
     (funcall ert--should-execution-observer form-description)))
 
+;; See Bug#24402 for why this exists
+(defun ert--signal-hook (error-symbol data)
+  "Stupid hack to stop `condition-case' from catching ert signals.
+It should only be stopped when ran from inside ert--run-test-internal."
+  (when (and (not (symbolp debugger))   ; only run on anonymous procedures
+             (memq error-symbol '(ert-test-failed ert-test-skipped)))
+    (funcall debugger 'error data)))
+
 (defun ert--special-operator-p (thing)
   "Return non-nil if THING is a symbol naming a special operator."
   (and (symbolp thing)
@@ -276,13 +284,15 @@ ert--special-operator-p
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         (condition-case err
+             (macroexpand-all form (append (bound-and-true-p
+                                            byte-compile-macro-environment)
+                                           (cond
+                                            ((boundp 'macroexpand-all-environment)
+                                             macroexpand-all-environment)
+                                            ((boundp 'cl-macro-environment)
+                                             cl-macro-environment))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -303,8 +313,13 @@ ert--expand-should-1
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (let ((signal-hook-function #'ert--signal-hook))
+                               (list ,@arg-forms))
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..b6a7eb68da 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -294,6 +294,15 @@ ert-self-test-and-exit
                   "the error signaled was a subtype of the expected type")))))
     ))
 
+(ert-deftest ert-test-should-error-argument ()
+  "Errors due to evaluating arguments should not break tests."
+  (should-error (identity (/ 1 0))))
+
+(ert-deftest ert-test-should-error-macroexpansion ()
+  "Errors due to expanding macros should not break tests."
+  (cl-macrolet ((test () (error "Foo")))
+    (should-error (test))))
+
 (ert-deftest ert-test-skip-unless ()
   ;; Don't skip.
   (let ((test (make-ert-test :body (lambda () (skip-unless t)))))

bug#24402: should-error doesn't catch all errors

[npostavs]

Alex <agrambot@gmail.com> writes:

> npostavs@users.sourceforge.net writes:
>
>> Yes, ert binds `debugger' in order to get full backtrace information
>> when there is an error.  This means it won't see errors caught by
>> condition-case.  That's good when it ignores errors caught by test code
>> using condition-case, but does give rise to problems.  There is some
>> relevant discussion in Bugs #11218 and #24617.
>>
>> Espcially the suggestion in #24617 of using `signal-hook-function' to
>> record error info instead of using `debugger', I think doing this could
>> simplify things a lot.  It is definitely going to require messing around
>> with ert's internals though...
>
> Thanks for the info. I may have discovered a workaround, but I'm not
> sure if there's any negative side-effects. All the tests pass, though.
>
> What do you think of it? It's obviously not ideal, but I think it at
> least fixes the issues at hand.

Does it also work when loading the elc version of the test file?  (try
'make check TEST_LOAD_EL=no')

What about tests like this?

    (ert-deftest check-error-handling ()
      (should
       (eq 42
           (condition-case ()
               (/ 1 0)
             (arith-error 42)))))

bug#24402: should-error doesn't catch all errors

[Alex]

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> npostavs@users.sourceforge.net writes:
>>
>>> Yes, ert binds `debugger' in order to get full backtrace information
>>> when there is an error.  This means it won't see errors caught by
>>> condition-case.  That's good when it ignores errors caught by test code
>>> using condition-case, but does give rise to problems.  There is some
>>> relevant discussion in Bugs #11218 and #24617.
>>>
>>> Espcially the suggestion in #24617 of using `signal-hook-function' to
>>> record error info instead of using `debugger', I think doing this could
>>> simplify things a lot.  It is definitely going to require messing around
>>> with ert's internals though...
>>
>> Thanks for the info. I may have discovered a workaround, but I'm not
>> sure if there's any negative side-effects. All the tests pass, though.
>>
>> What do you think of it? It's obviously not ideal, but I think it at
>> least fixes the issues at hand.
>
> Does it also work when loading the elc version of the test file?  (try
> 'make check TEST_LOAD_EL=no')

Oh, it doesn't load the elc version by default? That's surprising; I
think that should be documented in the test README.

I get 3 test failures with TEST_LOAD_EL=no, but I don't believe they're
because of me. On a mostly clean master (d014a5e15) those 3 also error.
One of them is simple to fix (the (require 'subr-x) should not be inside
eval-when-compile in dom-tests). The other failing tests are
subr-test-backtrace-integration-test and cl-lib-defstruct-record.

> What about tests like this?
>
>     (ert-deftest check-error-handling ()
>       (should
>        (eq 42
>            (condition-case ()
>                (/ 1 0)
>              (arith-error 42)))))

It works for me, yes. As long as `debugger' is set to a symbol. I can
make it a bit more robust by using an additional defvar in
ert--run-test-internal.

Are you asking because it doesn't work for you?

bug#24402: should-error doesn't catch all errors

[npostavs]

Alex <agrambot@gmail.com> writes:

> npostavs@users.sourceforge.net writes:
>
>> Does it also work when loading the elc version of the test file?  (try
>> 'make check TEST_LOAD_EL=no')
>
> Oh, it doesn't load the elc version by default? That's surprising; I
> think that should be documented in the test README.
>
> I get 3 test failures with TEST_LOAD_EL=no, but I don't believe they're
> because of me. On a mostly clean master (d014a5e15) those 3 also error.
> One of them is simple to fix (the (require 'subr-x) should not be inside
> eval-when-compile in dom-tests).

Ah, the `should' blocks inlining during compilation.  Is that necessary?
Probably yes if we expect to catch errors during macroexpansion I guess.

> The other failing tests are
> subr-test-backtrace-integration-test and cl-lib-defstruct-record.

Hmm, I'll see if I can fix these.

>> What about tests like this?
>>
>>     (ert-deftest check-error-handling ()
>>       (should
>>        (eq 42
>>            (condition-case ()
>>                (/ 1 0)
>>              (arith-error 42)))))
>
> It works for me, yes. As long as `debugger' is set to a symbol. I can
> make it a bit more robust by using an additional defvar in
> ert--run-test-internal.
>
> Are you asking because it doesn't work for you?

No, I'm just trying to explore the edges of this solution.  Isn't
`debugger' bound to a non-symbol while running the the tests?  I'm
confused as to why this solution works.

bug#24402: should-error doesn't catch all errors

[Alex]

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

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> npostavs@users.sourceforge.net writes:
>>
>>> Does it also work when loading the elc version of the test file?  (try
>>> 'make check TEST_LOAD_EL=no')
>>
>> Oh, it doesn't load the elc version by default? That's surprising; I
>> think that should be documented in the test README.
>>
>> I get 3 test failures with TEST_LOAD_EL=no, but I don't believe they're
>> because of me. On a mostly clean master (d014a5e15) those 3 also error.
>> One of them is simple to fix (the (require 'subr-x) should not be inside
>> eval-when-compile in dom-tests).
>
> Ah, the `should' blocks inlining during compilation.  Is that necessary?
> Probably yes if we expect to catch errors during macroexpansion I guess.

Can you get errors by expanding inlined functions?

Macros are expanded at compile-time with the current patch. If there are
any macroexpansion errors, then the form is altered to be (error <type>
<data>). Perhaps inline functions could work similarly.

Here's a diff to my patch that uses byte-compile-inline-expand. This
fixes the dom-tests case. Do you think it's worth it?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: inline --]
[-- Type: text/x-diff, Size: 2280 bytes --]

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index beb32c48ce..f4d2f725ac 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -281,17 +281,19 @@ ert--special-operator-p
          (and (subrp definition)
               (eql (cdr (subr-arity definition)) 'unevalled)))))
 
+(require 'byte-opt)
+
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
          (condition-case err
-             (macroexpand-all form (append (bound-and-true-p
-                                            byte-compile-macro-environment)
-                                           (cond
-                                            ((boundp 'macroexpand-all-environment)
-                                             macroexpand-all-environment)
-                                            ((boundp 'cl-macro-environment)
-                                             cl-macro-environment))))
+             (byte-compile-inline-expand (macroexpand-all form (append (bound-and-true-p
+                                                                        byte-compile-macro-environment)
+                                                                       (cond
+                                                                        ((boundp 'macroexpand-all-environment)
+                                                                         macroexpand-all-environment)
+                                                                        ((boundp 'cl-macro-environment)
+                                                                         cl-macro-environment)))))
            (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
@@ -308,7 +310,8 @@ ert--expand-should-1
         (cl-assert (or (symbolp fn-name)
                        (and (consp fn-name)
                             (eql (car fn-name) 'lambda)
-                            (listp (cdr fn-name)))))
+                            (listp (cdr fn-name)))
+                       (byte-code-function-p fn-name)))
         (let ((fn (cl-gensym "fn-"))
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))

[-- Attachment #3: Type: text/plain, Size: 1607 bytes --]


>> The other failing tests are
>> subr-test-backtrace-integration-test and cl-lib-defstruct-record.
>
> Hmm, I'll see if I can fix these.

Thanks. I noticed when byte-compiling cl-lib-tests, I got this warning:
Unused lexical variable ‘cl-struct-foo’.

>>> What about tests like this?
>>>
>>>     (ert-deftest check-error-handling ()
>>>       (should
>>>        (eq 42
>>>            (condition-case ()
>>>                (/ 1 0)
>>>              (arith-error 42)))))
>>
>> It works for me, yes. As long as `debugger' is set to a symbol. I can
>> make it a bit more robust by using an additional defvar in
>> ert--run-test-internal.
>>
>> Are you asking because it doesn't work for you?
>
> No, I'm just trying to explore the edges of this solution.  Isn't
> `debugger' bound to a non-symbol while running the the tests?  I'm
> confused as to why this solution works.

Yes, that's why there's the second test that checks for error-symbol to
be ert-test-{failed, skipped}. Basically what's happening is that
ert--signal-hook forces the debugger to trigger even inside a
condition-case, but only with a non-symbol `debugger' (since
ert--run-test-internal binds it to a closure), and one of the above two
errors.

The only time this approach fails is when you bind `debugger' to a
non-symbol and also signal ert-test-{failed, skipped}.

This is relatively rare compared to the problems at hand (macro and
argument errors), so unless there are unforeseen issues I think it's an
acceptable stop-gap solution. Hopefully Someone™ can properly fix this
eventually.

bug#24402: should-error doesn't catch all errors

[npostavs]

Alex <agrambot@gmail.com> writes:

>>> I get 3 test failures with TEST_LOAD_EL=no, but I don't believe they're
>>> because of me. On a mostly clean master (d014a5e15) those 3 also error.
>>> One of them is simple to fix (the (require 'subr-x) should not be inside
>>> eval-when-compile in dom-tests).
>>
>> Ah, the `should' blocks inlining during compilation.  Is that necessary?
>> Probably yes if we expect to catch errors during macroexpansion I guess.
>
> Can you get errors by expanding inlined functions?

Yes I think so, probably not from defsubst (except for wrong number of
arguments maybe?), but I believe define-inline basically lets you run an
arbitrary macro to produce the inlined call.

> Macros are expanded at compile-time with the current patch. If there are
> any macroexpansion errors, then the form is altered to be (error <type>
> <data>). Perhaps inline functions could work similarly.
>
> Here's a diff to my patch that uses byte-compile-inline-expand. This
> fixes the dom-tests case. Do you think it's worth it?

It would be nice if we can make code inside tests behave the same as
outside.  But we should make it conditional on whether the code is being
compiled, otherwise code inside tests would behave differently when
being interpreted.  Anyway, we can leave this for a separate bug.

> Yes, that's why there's the second test that checks for error-symbol to
> be ert-test-{failed, skipped}. Basically what's happening is that
> ert--signal-hook forces the debugger to trigger even inside a
> condition-case, but only with a non-symbol `debugger' (since
> ert--run-test-internal binds it to a closure), and one of the above two
> errors.
>
> The only time this approach fails is when you bind `debugger' to a
> non-symbol and also signal ert-test-{failed, skipped}.

Okay, it sounds like we would only hit problems when running an ert test
from inside an ert test.  That should be pretty rare outside of ert's
test suite, and we already have workarounds for that case, right?

> This is relatively rare compared to the problems at hand (macro and
> argument errors), so unless there are unforeseen issues I think it's an
> acceptable stop-gap solution. Hopefully Someone™ can properly fix this
> eventually.

Yes, I think this approach is acceptable.

bug#24402: should-error doesn't catch all errors

[Alex]

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

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> Macros are expanded at compile-time with the current patch. If there are
>> any macroexpansion errors, then the form is altered to be (error <type>
>> <data>). Perhaps inline functions could work similarly.
>>
>> Here's a diff to my patch that uses byte-compile-inline-expand. This
>> fixes the dom-tests case. Do you think it's worth it?
>
> It would be nice if we can make code inside tests behave the same as
> outside.  But we should make it conditional on whether the code is being
> compiled, otherwise code inside tests would behave differently when
> being interpreted.  Anyway, we can leave this for a separate bug.

I agree, but that sounds like it'll require a fair bit of refactoring
and knowledge of ert internals.

OOC, is there a robust way to check whether or not you're currently
byte-compiling?

>> Yes, that's why there's the second test that checks for error-symbol to
>> be ert-test-{failed, skipped}. Basically what's happening is that
>> ert--signal-hook forces the debugger to trigger even inside a
>> condition-case, but only with a non-symbol `debugger' (since
>> ert--run-test-internal binds it to a closure), and one of the above two
>> errors.
>>
>> The only time this approach fails is when you bind `debugger' to a
>> non-symbol and also signal ert-test-{failed, skipped}.
>
> Okay, it sounds like we would only hit problems when running an ert test
> from inside an ert test.  That should be pretty rare outside of ert's
> test suite, and we already have workarounds for that case, right?

I tried a nested case using two ert-deftests and it worked, so it looks
okay here too.

>> This is relatively rare compared to the problems at hand (macro and
>> argument errors), so unless there are unforeseen issues I think it's an
>> acceptable stop-gap solution. Hopefully Someone™ can properly fix this
>> eventually.
>
> Yes, I think this approach is acceptable.

I was going to ask if you would merge in a few days, but it appears that
what should have been a simple rebase to master caused unforeseen
consequences. For instance, for some reason I now get a segmentation
fault when executing 'make cl-lib-tests TEST_LOAD_EL=no'. I even reset
to the commit I was at before and it still segfaults. Can you reproduce
this with the following patch on master?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ert --]
[-- Type: text/x-diff, Size: 6080 bytes --]

From fdbf66f73672d9b69fc37273223ae90fff951eb2 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Thu, 13 Jul 2017 14:54:35 -0600
Subject: [PATCH] Catch argument and expansion errors in ert

This kludge catches errors caused by evaluating arguments in ert's
should, should-not, and should-error macros; it also catches macro and
inline function expansion errors inside of the above
macros (Bug#24402).

* lisp/emacs-lisp/ert.el: (ert--should-signal-hook): New function.
(ert--expand-should-1): Catch expansion errors.
* test/lisp/emacs-lisp/ert-tests.el (ert-test-should-error-argument)
(ert-test-should-error-macroexpansion): Tests for argument and
expansion errors.
---
 lisp/emacs-lisp/ert.el            | 47 ++++++++++++++++++++++++++++++---------
 test/lisp/emacs-lisp/ert-tests.el |  9 ++++++++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..c6e3ee8503 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -65,6 +65,7 @@
 (require 'find-func)
 (require 'help)
 (require 'pp)
+(require 'byte-opt) ; for ert--expand-should-1 kludge
 
 ;;; UI customization options.
 
@@ -266,6 +267,14 @@ ert--signal-should-execution
   (when ert--should-execution-observer
     (funcall ert--should-execution-observer form-description)))
 
+;; See Bug#24402 for why this exists
+(defun ert--should-signal-hook (error-symbol data)
+  "Stupid hack to stop `condition-case' from catching ert signals.
+It should only be stopped when ran from inside ert--run-test-internal."
+  (when (and (not (symbolp debugger))   ; only run on anonymous debugger
+             (memq error-symbol '(ert-test-failed ert-test-skipped)))
+    (funcall debugger 'error data)))
+
 (defun ert--special-operator-p (thing)
   "Return non-nil if THING is a symbol naming a special operator."
   (and (symbolp thing)
@@ -276,13 +285,20 @@ ert--special-operator-p
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         ;; catch all macro and inline function expansion errors
+         ;; FIXME: Code inside of tests should be evaluated like it is
+         ;; outside of tests, with the sole exception of error
+         ;; handling
+         (condition-case err
+             (byte-compile-inline-expand
+              (macroexpand-all form (append (bound-and-true-p
+                                             byte-compile-macro-environment)
+                                            (cond
+                                             ((boundp 'macroexpand-all-environment)
+                                              macroexpand-all-environment)
+                                             ((boundp 'cl-macro-environment)
+                                              cl-macro-environment)))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -298,13 +314,20 @@ ert--expand-should-1
         (cl-assert (or (symbolp fn-name)
                        (and (consp fn-name)
                             (eql (car fn-name) 'lambda)
-                            (listp (cdr fn-name)))))
+                            (listp (cdr fn-name)))
+                       ;; for inline functions
+                       (byte-code-function-p fn-name)))
         (let ((fn (cl-gensym "fn-"))
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (let ((signal-hook-function #'ert--should-signal-hook))
+                               (list ,@arg-forms))
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
@@ -766,6 +789,10 @@ ert--run-test-internal
     ;; too expensive, we can remove it.
     (with-temp-buffer
       (save-window-excursion
+        ;; FIXME: Use `signal-hook-function' instead of `debugger' to
+        ;; handle ert errors. Once that's done, remove
+        ;; `ert--should-signal-hook'. See Bug#24402 and Bug#11218 for
+        ;; details.
         (let ((debugger (lambda (&rest args)
                           (ert--run-test-debugger test-execution-info
                                                   args)))
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..b6a7eb68da 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -294,6 +294,15 @@ ert-self-test-and-exit
                   "the error signaled was a subtype of the expected type")))))
     ))
 
+(ert-deftest ert-test-should-error-argument ()
+  "Errors due to evaluating arguments should not break tests."
+  (should-error (identity (/ 1 0))))
+
+(ert-deftest ert-test-should-error-macroexpansion ()
+  "Errors due to expanding macros should not break tests."
+  (cl-macrolet ((test () (error "Foo")))
+    (should-error (test))))
+
 (ert-deftest ert-test-skip-unless ()
   ;; Don't skip.
   (let ((test (make-ert-test :body (lambda () (skip-unless t)))))
-- 
2.13.2

bug#24402: should-error doesn't catch all errors

[npostavs]

Alex <agrambot@gmail.com> writes:

> npostavs@users.sourceforge.net writes:
>> It would be nice if we can make code inside tests behave the same as
>> outside.  But we should make it conditional on whether the code is being
>> compiled, otherwise code inside tests would behave differently when
>> being interpreted.  Anyway, we can leave this for a separate bug.
>
> I agree, but that sounds like it'll require a fair bit of refactoring
> and knowledge of ert internals.

I don't think so, just a conditional to decide whether or not to call
the extra expansion.  Do you think there is anything else?

> OOC, is there a robust way to check whether or not you're currently
> byte-compiling?

AFAIK, the usual trick is (bound-and-true-p byte-compile-current-file).
It's probably good enough for most things.

> I was going to ask if you would merge in a few days, but it appears that
> what should have been a simple rebase to master caused unforeseen
> consequences. For instance, for some reason I now get a segmentation
> fault when executing 'make cl-lib-tests TEST_LOAD_EL=no'. I even reset
> to the commit I was at before and it still segfaults. Can you reproduce
> this with the following patch on master?

Nope, I just get the failures on cl-lib-defstruct-record we already
mentioned.

bug#24402: should-error doesn't catch all errors

[Alex]

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> npostavs@users.sourceforge.net writes:
>>> It would be nice if we can make code inside tests behave the same as
>>> outside.  But we should make it conditional on whether the code is being
>>> compiled, otherwise code inside tests would behave differently when
>>> being interpreted.  Anyway, we can leave this for a separate bug.
>>
>> I agree, but that sounds like it'll require a fair bit of refactoring
>> and knowledge of ert internals.
>
> I don't think so, just a conditional to decide whether or not to call
> the extra expansion.  Do you think there is anything else?

I was mostly referring to not binding `debugger', but also evaluating
the code "normally" (i.e., not doing expansions first in one
condition-case, evaluating arguments in another, and then the whole form
in a third one).

>> OOC, is there a robust way to check whether or not you're currently
>> byte-compiling?
>
> AFAIK, the usual trick is (bound-and-true-p byte-compile-current-file).
> It's probably good enough for most things.

I believe the below patch does that, though it has some issues.

>> I was going to ask if you would merge in a few days, but it appears that
>> what should have been a simple rebase to master caused unforeseen
>> consequences. For instance, for some reason I now get a segmentation
>> fault when executing 'make cl-lib-tests TEST_LOAD_EL=no'. I even reset
>> to the commit I was at before and it still segfaults. Can you reproduce
>> this with the following patch on master?
>
> Nope, I just get the failures on cl-lib-defstruct-record we already
> mentioned.

The segfault appears to have been because I didn't wipe out the elc
files when testing different implementations.

I spent a lot longer than I'd like to admit finding this out. Is there a
reason why "make clean" in the test directory doesn't wipe out elc
files? I don't understand why there's a separate bootstrap-clean that
does this. Can this and TEST_LOAD_EL please be documented in the test
README?

Anyway, I got everything back in order. Sadly, there's a couple extra
tests that now fail for me in the patch that *doesn't* expand inline
functions, and these don't fail for me in a clean master. They are in
eieio-tests (23 and 24).

With the inline expansion, I also get some errors in ert-tests. All of
the errors, with the exception of subr-tests error, seem to be from
cl-defstruct and cl-typep (which is defined by define-inline).

Do you have any ideas? There should be 5 unexpected errors without the
inline expansion, and 6 errors with it. Note that all tests pass in both
cases without "TEST_LOAD_EL=no".

If it's easy to fix the eieio tests and not the other ones, then it
might be better to leave the inline-function expansion out for now.

bug#24402: should-error doesn't catch all errors

[Alex]

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

Alex <agrambot@gmail.com> writes:

> I believe the below patch does that, though it has some issues.

Here's the patch, sorry:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Catch-argument-and-expansion-errors-in-ert.patch --]
[-- Type: text/x-diff, Size: 6354 bytes --]

From ba2703bce46e7caa6e86d7406a712bd92d06f84d Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Thu, 13 Jul 2017 14:54:35 -0600
Subject: [PATCH] Catch argument and expansion errors in ert

This kludge catches errors caused by evaluating arguments in ert's
should, should-not, and should-error macros; it also catches macro and
inline function expansion errors inside of the above
macros (Bug#24402).

* lisp/emacs-lisp/ert.el: (ert--should-signal-hook): New function.
(ert--expand-should-1): Catch expansion errors.
* test/lisp/emacs-lisp/ert-tests.el (ert-test-should-error-argument)
(ert-test-should-error-macroexpansion): Tests for argument and
expansion errors.
---
 lisp/emacs-lisp/ert.el            | 53 +++++++++++++++++++++++++++++++--------
 test/lisp/emacs-lisp/ert-tests.el |  9 +++++++
 2 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..353cbb8d57 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -266,6 +266,14 @@ DATA is displayed to the user and should state the reason for skipping."
   (when ert--should-execution-observer
     (funcall ert--should-execution-observer form-description)))
 
+;; See Bug#24402 for why this exists
+(defun ert--should-signal-hook (error-symbol data)
+  "Stupid hack to stop `condition-case' from catching ert signals.
+It should only be stopped when ran from inside ert--run-test-internal."
+  (when (and (not (symbolp debugger))   ; only run on anonymous debugger
+             (memq error-symbol '(ert-test-failed ert-test-skipped)))
+    (funcall debugger 'error data)))
+
 (defun ert--special-operator-p (thing)
   "Return non-nil if THING is a symbol naming a special operator."
   (and (symbolp thing)
@@ -276,13 +284,27 @@ DATA is displayed to the user and should state the reason for skipping."
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         ;; catch all macro and inline function expansion errors
+         ;; FIXME: Code inside of tests should be evaluated like it is
+         ;; outside of tests, with the sole exception of error
+         ;; handling
+         (condition-case err
+             (funcall
+              ;; expand only when byte-compiling
+              (lambda (form)
+                (if (and (bound-and-true-p byte-compile-current-file)
+                         (consp form))
+                    (byte-compile-inline-expand form)
+                  form))
+              (macroexpand-all form
+                               (append (bound-and-true-p
+                                        byte-compile-macro-environment)
+                                       (cond
+                                        ((boundp 'macroexpand-all-environment)
+                                         macroexpand-all-environment)
+                                        ((boundp 'cl-macro-environment)
+                                         cl-macro-environment)))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -298,13 +320,20 @@ DATA is displayed to the user and should state the reason for skipping."
         (cl-assert (or (symbolp fn-name)
                        (and (consp fn-name)
                             (eql (car fn-name) 'lambda)
-                            (listp (cdr fn-name)))))
+                            (listp (cdr fn-name)))
+                       ;; for inline functions
+                       (byte-code-function-p fn-name)))
         (let ((fn (cl-gensym "fn-"))
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (let ((signal-hook-function #'ert--should-signal-hook))
+                               (list ,@arg-forms))
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
@@ -766,6 +795,10 @@ This mainly sets up debugger-related bindings."
     ;; too expensive, we can remove it.
     (with-temp-buffer
       (save-window-excursion
+        ;; FIXME: Use `signal-hook-function' instead of `debugger' to
+        ;; handle ert errors. Once that's done, remove
+        ;; `ert--should-signal-hook'. See Bug#24402 and Bug#11218 for
+        ;; details.
         (let ((debugger (lambda (&rest args)
                           (ert--run-test-debugger test-execution-info
                                                   args)))
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..b6a7eb68da 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -294,6 +294,15 @@ failed or if there was a problem."
                   "the error signaled was a subtype of the expected type")))))
     ))
 
+(ert-deftest ert-test-should-error-argument ()
+  "Errors due to evaluating arguments should not break tests."
+  (should-error (identity (/ 1 0))))
+
+(ert-deftest ert-test-should-error-macroexpansion ()
+  "Errors due to expanding macros should not break tests."
+  (cl-macrolet ((test () (error "Foo")))
+    (should-error (test))))
+
 (ert-deftest ert-test-skip-unless ()
   ;; Don't skip.
   (let ((test (make-ert-test :body (lambda () (skip-unless t)))))
-- 
2.13.2

bug#24402: should-error doesn't catch all errors

[npostavs]

Alex <agrambot@gmail.com> writes:

> The segfault appears to have been because I didn't wipe out the elc
> files when testing different implementations.

I suspect getting a segfault might indicate an actual bug somewhere.

> I spent a lot longer than I'd like to admit finding this out. Is there a
> reason why "make clean" in the test directory doesn't wipe out elc
> files? I don't understand why there's a separate bootstrap-clean that
> does this. Can this and TEST_LOAD_EL please be documented in the test
> README?

I think it was basically copied from the other Makefiles, where cleaning
all elc files would mean a very long subsequent compilation.  It might
make sense to break the pattern for the test/ subdirectory though.

> Anyway, I got everything back in order. Sadly, there's a couple extra
> tests that now fail for me in the patch that *doesn't* expand inline
> functions, and these don't fail for me in a clean master. They are in
> eieio-tests (23 and 24).

I'm seeing eieio-tests failing also in master.  This seems to be an
actual bug, in the definition of `cl-typep' I think.  I've opened a new
bug for this (Bug#27718).

> With the inline expansion, I also get some errors in ert-tests. All of
> the errors, with the exception of subr-tests error, seem to be from
> cl-defstruct and cl-typep (which is defined by define-inline).
>
> Do you have any ideas? There should be 5 unexpected errors without the
> inline expansion, and 6 errors with it. Note that all tests pass in both
> cases without "TEST_LOAD_EL=no".
>
> If it's easy to fix the eieio tests and not the other ones, then it
> might be better to leave the inline-function expansion out for now.

I have a fix for the subr-tests failed, as for the others, I don't know
enough about the compilation process to untangle it yet.  I think we
should just leave the inline-function expansion part out for now, at
which point I believe your patch won't be making anything worse, so it
should be okay to install.

bug#24402: should-error doesn't catch all errors

[Alex]

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

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> The segfault appears to have been because I didn't wipe out the elc
>> files when testing different implementations.
>
> I suspect getting a segfault might indicate an actual bug somewhere.

Probably, but I can't reproduce it anymore since I got rid of the
offending .elc files.

>> I spent a lot longer than I'd like to admit finding this out. Is there a
>> reason why "make clean" in the test directory doesn't wipe out elc
>> files? I don't understand why there's a separate bootstrap-clean that
>> does this. Can this and TEST_LOAD_EL please be documented in the test
>> README?
>
> I think it was basically copied from the other Makefiles, where cleaning
> all elc files would mean a very long subsequent compilation.  It might
> make sense to break the pattern for the test/ subdirectory though.

That makes sense to me, but if others disagree then the behaviour being
documented is the next best thing.

>> Anyway, I got everything back in order. Sadly, there's a couple extra
>> tests that now fail for me in the patch that *doesn't* expand inline
>> functions, and these don't fail for me in a clean master. They are in
>> eieio-tests (23 and 24).
>
> I'm seeing eieio-tests failing also in master.  This seems to be an
> actual bug, in the definition of `cl-typep' I think.  I've opened a new
> bug for this (Bug#27718).

Oddly enough, I can't reproduce this on master. I cloned a new copy, ran
"./autogen.sh && ./configure && make -j4", then ran "make eieio-tests
TEST_LOAD_EL=no" with no error. I cloned from 30444c695, then tried
again with 7a0fb008. I also tried "make check-expensive TEST_LOAD_EL=no"
and got only 2 errors (dom-tests and cl-lib-tests).

Perhaps odder is that I can still reproduce your recipe in Bug#27718 in
that same repository.

>> With the inline expansion, I also get some errors in ert-tests. All of
>> the errors, with the exception of subr-tests error, seem to be from
>> cl-defstruct and cl-typep (which is defined by define-inline).
>>
>> Do you have any ideas? There should be 5 unexpected errors without the
>> inline expansion, and 6 errors with it. Note that all tests pass in both
>> cases without "TEST_LOAD_EL=no".
>>
>> If it's easy to fix the eieio tests and not the other ones, then it
>> might be better to leave the inline-function expansion out for now.
>
> I have a fix for the subr-tests failed, as for the others, I don't know
> enough about the compilation process to untangle it yet.  I think we
> should just leave the inline-function expansion part out for now, at
> which point I believe your patch won't be making anything worse, so it
> should be okay to install.

Sounds good. It would be lucky if a fix to Bug#27718 also resolves the
other inline function errors so that we could use the previous patch.
Here's an updated patch with inline function excluded:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Catch-argument-and-macroexpansion-errors-in-ert.patch --]
[-- Type: text/x-diff, Size: 5700 bytes --]

From 05f0734dd0b066058d263756a2e98cafc4ef8515 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Thu, 13 Jul 2017 14:54:35 -0600
Subject: [PATCH] Catch argument and macroexpansion errors in ert

This kludge catches errors caused by evaluating arguments in ert's
should, should-not, and should-error macros; it also catches
macroexpansion errors inside of the above macros (Bug#24402).

* lisp/emacs-lisp/ert.el: (ert--should-signal-hook): New function.
(ert--expand-should-1): Catch macroexpansion errors.
* test/lisp/emacs-lisp/ert-tests.el (ert-test-should-error-argument)
(ert-test-should-error-macroexpansion): Tests for argument and
expansion errors.
---
 lisp/emacs-lisp/ert.el            | 41 ++++++++++++++++++++++++++++++---------
 test/lisp/emacs-lisp/ert-tests.el |  9 +++++++++
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..caa4a9f389 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -266,6 +266,14 @@ DATA is displayed to the user and should state the reason for skipping."
   (when ert--should-execution-observer
     (funcall ert--should-execution-observer form-description)))
 
+;; See Bug#24402 for why this exists
+(defun ert--should-signal-hook (error-symbol data)
+  "Stupid hack to stop `condition-case' from catching ert signals.
+It should only be stopped when ran from inside ert--run-test-internal."
+  (when (and (not (symbolp debugger))   ; only run on anonymous debugger
+             (memq error-symbol '(ert-test-failed ert-test-skipped)))
+    (funcall debugger 'error data)))
+
 (defun ert--special-operator-p (thing)
   "Return non-nil if THING is a symbol naming a special operator."
   (and (symbolp thing)
@@ -273,16 +281,22 @@ DATA is displayed to the user and should state the reason for skipping."
          (and (subrp definition)
               (eql (cdr (subr-arity definition)) 'unevalled)))))
 
+;; FIXME: Code inside of here should probably be evaluated like it is
+;; outside of tests, with the sole exception of error handling
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         ;; catch macroexpansion errors
+         (condition-case err
+             (macroexpand-all form
+                              (append (bound-and-true-p
+                                       byte-compile-macro-environment)
+                                      (cond
+                                       ((boundp 'macroexpand-all-environment)
+                                        macroexpand-all-environment)
+                                       ((boundp 'cl-macro-environment)
+                                        cl-macro-environment))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -303,8 +317,13 @@ DATA is displayed to the user and should state the reason for skipping."
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (let ((signal-hook-function #'ert--should-signal-hook))
+                               (list ,@arg-forms))
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
@@ -766,6 +785,10 @@ This mainly sets up debugger-related bindings."
     ;; too expensive, we can remove it.
     (with-temp-buffer
       (save-window-excursion
+        ;; FIXME: Use `signal-hook-function' instead of `debugger' to
+        ;; handle ert errors. Once that's done, remove
+        ;; `ert--should-signal-hook'. See Bug#24402 and Bug#11218 for
+        ;; details.
         (let ((debugger (lambda (&rest args)
                           (ert--run-test-debugger test-execution-info
                                                   args)))
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..b6a7eb68da 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -294,6 +294,15 @@ failed or if there was a problem."
                   "the error signaled was a subtype of the expected type")))))
     ))
 
+(ert-deftest ert-test-should-error-argument ()
+  "Errors due to evaluating arguments should not break tests."
+  (should-error (identity (/ 1 0))))
+
+(ert-deftest ert-test-should-error-macroexpansion ()
+  "Errors due to expanding macros should not break tests."
+  (cl-macrolet ((test () (error "Foo")))
+    (should-error (test))))
+
 (ert-deftest ert-test-skip-unless ()
   ;; Don't skip.
   (let ((test (make-ert-test :body (lambda () (skip-unless t)))))
-- 
2.13.2

bug#24402: should-error doesn't catch all errors

[npostavs]

Alex <agrambot@gmail.com> writes:

> Oddly enough, I can't reproduce this on master. I cloned a new copy, ran
> "./autogen.sh && ./configure && make -j4", then ran "make eieio-tests
> TEST_LOAD_EL=no" with no error. I cloned from 30444c695, then tried
> again with 7a0fb008. I also tried "make check-expensive TEST_LOAD_EL=no"
> and got only 2 errors (dom-tests and cl-lib-tests).
>
> Perhaps odder is that I can still reproduce your recipe in Bug#27718 in
> that same repository.

Uh, actually, now I'm getting the same results as you (i.e., make
eieio-tests TEST_LOAD_EL=no passes on master for me, though the #27718
test case does fail).  Perhaps I accidentally ran an eieio-tests.elc
file produced by a patched Emacs using a non-patched Emacs before?

>> I have a fix for the subr-tests failed, as for the others, I don't know
>> enough about the compilation process to untangle it yet.  I think we
>> should just leave the inline-function expansion part out for now, at
>> which point I believe your patch won't be making anything worse, so it
>> should be okay to install.
>
> Sounds good. It would be lucky if a fix to Bug#27718 also resolves the
> other inline function errors so that we could use the previous patch.
> Here's an updated patch with inline function excluded:

Okay, so the patch does make a couple more tests fail, but I think it's
actually an improvement since #27718 shows that the scenario was already
failing outside of ert-deftest, i.e., in this case your patch is making
code inside the test more like code outside the test.  We can just mark
those as expected failures when compiled.

I will merge to master in a few days, assuming there are no objections.

bug#24402: should-error doesn't catch all errors

[Gemini Lasswell]

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

Hello,

First, thank you for working on this bug!

I have a branch of Emacs which attempts to run all the tests under
Testcover, and I've tried it with Alex's latest patch. The patch has
fixed the problems with Testcover and should-error except for one weird
case. Here is a file that will let you reproduce the weird case on
master with the patch applied:


[-- Attachment #2: cyc-test.el --]
[-- Type: text/plain, Size: 449 bytes --]

(defun cyc1 (a)
  (let ((ls (make-list 10 a)))
    (nconc ls ls)
    ls))

(ert-deftest test-cycle-assq ()
  (let ((c1 (cyc1 '(1))))
    (should-error (assq 3 c1) :type 'circular-list)))

(ert-deftest test-cycle-assoc ()
  (let ((c1 (cyc1 '(1))))
    (should-error (assoc 2 c1) :type 'circular-list)))

(defvar testcover-started nil)
(unless testcover-started
  (setq testcover-started t)
  (testcover-start (or load-file-name (buffer-file-name))))

[-- Attachment #3: Type: text/plain, Size: 414 bytes --]


This is an excerpt of test/src/fns-tests.el with a few lines added at
the end to invoke Testcover. It contains two nearly identical tests
which should both pass, but one passes and one fails. If you then edit
the file and comment out test-cycle-assoc (the one that passes) and run
the test again, then the failing test will pass. I've reproduced it both
in batch mode (without TEST_LOAD_EL=no) and interactively.

bug#24402: should-error doesn't catch all errors

[Alex]

Gemini Lasswell <gazally@runbox.com> writes:

> Hello,
>
> First, thank you for working on this bug!
>
> I have a branch of Emacs which attempts to run all the tests under
> Testcover, and I've tried it with Alex's latest patch. The patch has
> fixed the problems with Testcover and should-error except for one weird
> case. Here is a file that will let you reproduce the weird case on
> master with the patch applied:
>
>
>
> This is an excerpt of test/src/fns-tests.el with a few lines added at
> the end to invoke Testcover. It contains two nearly identical tests
> which should both pass, but one passes and one fails. If you then edit
> the file and comment out test-cycle-assoc (the one that passes) and run
> the test again, then the failing test will pass. I've reproduced it both
> in batch mode (without TEST_LOAD_EL=no) and interactively.

Hmm, this isn't the behaviour that I see. I get the following errors
both with and without my patch, and both interactively and in batch
mode:

===============================================================================================
Edebug: cyc1
Edebug: test@test-cycle-assq
Edebug: test@test-cycle-assoc
Edebug: cyc1
Edebug: test@test-cycle-assq
Edebug: test@test-cycle-assoc
...
Edebug: cyc1
Edebug: test@test-cycle-assq
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Edebug: test@test-cycle-assoc
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Edebug: cyc1
Edebug: test@test-cycle-assq
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Eager macro-expansion failure: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Eager macro-expansion failure: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Entering debugger...
cl--generic-make-next-function: Symbol’s function definition is void: t
===============================================================================================

The ... above represents Edebug cycling, for some reason, between the 3
functions.

Noam, can you reproduce this?

bug#24402: should-error doesn't catch all errors

[npostavs]

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

Gemini Lasswell <gazally@runbox.com> writes:

> This is an excerpt of test/src/fns-tests.el with a few lines added at
> the end to invoke Testcover. It contains two nearly identical tests
> which should both pass, but one passes and one fails.

If I run them again in the same Emacs session then they both fail.  The
problem seems to be that testcover saves values produced in tests, and
when it tries to compare the 2 circular lists produced by these tests, a
`circular-list' error is thrown.  In other words, this is actually a
totally separate bug (although it's hidden until a fix for #24402 is
applied).

The following appears to fix it, though perhaps we should use a smarter
equal function that would consider the circular lists to actually be
equal instead of bailing out and returning nil on circularity.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1501 bytes --]

From 7160e8e45ef9e074360e394f99120d11b7ed4b8a Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 19 Jul 2017 18:48:50 -0400
Subject: [PATCH v1] Don't error on circular values in testcover

* lisp/emacs-lisp/testcover.el (testcover-after, testcover-1value):
Consider circular lists to be non-equal instead of signaling error.
---
 lisp/emacs-lisp/testcover.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/testcover.el b/lisp/emacs-lisp/testcover.el
index 433ad38a14..bc6004a709 100644
--- a/lisp/emacs-lisp/testcover.el
+++ b/lisp/emacs-lisp/testcover.el
@@ -463,7 +463,9 @@ (defun testcover-after (idx val)
   (cond
    ((eq (aref testcover-vector idx) 'unknown)
     (aset testcover-vector idx val))
-   ((not (equal (aref testcover-vector idx) val))
+   ((not (condition-case ()
+             (equal (aref testcover-vector idx) val)
+           (circular-list nil)))
     (aset testcover-vector idx 'ok-coverage)))
   val)
 
@@ -475,7 +477,9 @@ (defun testcover-1value (idx val)
    ((eq (aref testcover-vector idx) '1value)
     (aset testcover-vector idx (cons '1value val)))
    ((not (and (eq (car-safe (aref testcover-vector idx)) '1value)
-	      (equal (cdr (aref testcover-vector idx)) val)))
+	      (condition-case ()
+                  (equal (cdr (aref testcover-vector idx)) val)
+                (circular-list nil))))
     (error "Value of form marked with `1value' does vary: %s" val)))
   val)
 
-- 
2.11.1


[-- Attachment #3: Type: text/plain, Size: 745 bytes --]


Alex <agrambot@gmail.com> writes:

> Hmm, this isn't the behaviour that I see. I get the following errors
> both with and without my patch, and both interactively and in batch
> mode:
>
> ===============================================================================================
> Edebug: cyc1
> Edebug: test@test-cycle-assq
> Edebug: test@test-cycle-assoc
> Edebug: cyc1
> Edebug: test@test-cycle-assq
> Edebug: test@test-cycle-assoc
> ...
> Edebug: cyc1
> Edebug: test@test-cycle-assq
> Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")

Hmm, that's strange, I don't get that error (or any "Edebug: cyc1"
repetitions after the first) on master, or with your patch.

bug#24402: should-error doesn't catch all errors

[Alex]

npostavs@users.sourceforge.net writes:

> Gemini Lasswell <gazally@runbox.com> writes:
>
>> This is an excerpt of test/src/fns-tests.el with a few lines added at
>> the end to invoke Testcover. It contains two nearly identical tests
>> which should both pass, but one passes and one fails.
>
> If I run them again in the same Emacs session then they both fail.  The
> problem seems to be that testcover saves values produced in tests, and
> when it tries to compare the 2 circular lists produced by these tests, a
> `circular-list' error is thrown.  In other words, this is actually a
> totally separate bug (although it's hidden until a fix for #24402 is
> applied).
>
> The following appears to fix it, though perhaps we should use a smarter
> equal function that would consider the circular lists to actually be
> equal instead of bailing out and returning nil on circularity.

That would be nice.

> Alex <agrambot@gmail.com> writes:
> 
> > Hmm, this isn't the behaviour that I see. I get the following errors
> > both with and without my patch, and both interactively and in batch
> > mode:
> >
> > ===============================================================================================
> > Edebug: cyc1
> > Edebug: test@test-cycle-assq
> > Edebug: test@test-cycle-assoc
> > Edebug: cyc1
> > Edebug: test@test-cycle-assq
> > Edebug: test@test-cycle-assoc
> > ...
> > Edebug: cyc1
> > Edebug: test@test-cycle-assq
> > Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
> 
> Hmm, that's strange, I don't get that error (or any "Edebug: cyc1"
> repetitions after the first) on master, or with your patch.

Whoops, I changed the defvar to a setq and forgot to change it back.
Doing this results in the error I posted. (Which makes sense since it
keeps reloading the file, resulting in the "Edebug:" loop. The
compiler-macro errors and `t' being called as a function are a bit odd,
though).

I get the same behaviour as you and Gemini now. Sorry about the noise.

bug#24402: should-error doesn't catch all errors

[Gemini Lasswell]

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

npostavs@users.sourceforge.net writes:

> The following appears to fix it, though perhaps we should use a smarter
> equal function that would consider the circular lists to actually be
> equal instead of bailing out and returning nil on circularity.

Thanks for tracking this one down and making a patch, which looks good
to me. Does that smarter equal function already exist? It doesn't seem
worth the effort of writing one for this purpose only.

Here is a test to add to your patch, which fails without it and passes
with it:


[-- Attachment #2: 0001-Add-a-test-of-handling-of-circular-values-to-testcov.patch --]
[-- Type: text/plain, Size: 1206 bytes --]

From f401e3169ff3887b8215b6625d111d70e5340efe Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Thu, 20 Jul 2017 12:01:42 -0700
Subject: [PATCH] Add a test of handling of circular values to testcover-tests

* test/lisp/emacs-lisp-testcover-resources/testcases.el
(testcover-testcase-cyc1): New function.
(testcover-tests-circular-lists-bug-24402): New test.
---
 test/lisp/emacs-lisp/testcover-resources/testcases.el | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/test/lisp/emacs-lisp/testcover-resources/testcases.el b/test/lisp/emacs-lisp/testcover-resources/testcases.el
index 1eb791a993..c9a5a6daac 100644
--- a/test/lisp/emacs-lisp/testcover-resources/testcases.el
+++ b/test/lisp/emacs-lisp/testcover-resources/testcases.el
@@ -490,4 +490,14 @@ testcover-testcase-how-do-i-know-you
 
 (should (eq (testcover-testcase-how-do-i-know-you "Liz") 'unknown))
 
+;; ==== circular-lists-bug-24402 ====
+"Testcover captures and ignores circular list errors."
+;; ====
+(defun testcover-testcase-cyc1 (a)
+  (let ((ls (make-list 10 a%%%)))
+    (nconc ls ls)
+    ls))
+(testcover-testcase-cyc1 1)
+(testcover-testcase-cyc1 1)
+
 ;; testcases.el ends here.
-- 
2.12.2

bug#24402: should-error doesn't catch all errors

[npostavs]

tags 24402 fixed
close 24402 26.1
quit

Sorry for the delay, I was moving apartment, and then I got a bit
distracted by my 'grep --null' mess.

Gemini Lasswell <gazally@runbox.com> writes:

> npostavs@users.sourceforge.net writes:
>
>> The following appears to fix it, though perhaps we should use a smarter
>> equal function that would consider the circular lists to actually be
>> equal instead of bailing out and returning nil on circularity.
>
> Thanks for tracking this one down and making a patch, which looks good
> to me. Does that smarter equal function already exist? It doesn't seem
> worth the effort of writing one for this purpose only.

Yeah, I left it as is with a TODO.  We can fix it later if it turns out
to cause an actual problem.

> Here is a test to add to your patch, which fails without it and passes
> with it:

Thanks, I pushed it to master along with the others.

[1: 054c198c12]: 2017-08-07 18:43:54 -0400
  Catch argument and macroexpansion errors in ert
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=054c198c120c1f01a8ff753892d52710b740acc6

[2: 95a04fd26c]: 2017-08-07 18:43:55 -0400
  ; Avoid test failures when running from compiled test files
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=95a04fd26c91e6c6c9191a629d26886f136e30fc

[3: 0508045ed7]: 2017-08-07 18:54:44 -0400
  Don't error on circular values in testcover
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=0508045ed7159bce5b5ea3b5fb72cf78b8b4ee8e

[4: 00f7e31110]: 2017-08-07 18:54:48 -0400
  Add a test of handling of circular values to testcover-tests
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=00f7e31110a27e568529192d7441d9631b9096bc