* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
@ 2016-11-13 22:22 Gemini Lasswell
2016-11-14 15:50 ` Eli Zaretskii
2016-11-14 16:18 ` Phillip Lord
0 siblings, 2 replies; 8+ messages in thread
From: Gemini Lasswell @ 2016-11-13 22:22 UTC (permalink / raw)
To: 24940
[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]
Hello,
In the process of writing tests for kmacro.el, I wrote two macros,
should-call and should-not-call, that create context by temporarily
adding advice to named functions. They allow a test writer to express
expectations of how functions are used, to mock up responses of those
functions to the code under test, and to prevent functions from running
which might modify the global state of Emacs in an undesirable way
during a test.
I think that these macros would be useful additions to ERT. Here is a
patch containing versions of the macros which are integrated into ERT
and which provide better failure reporting than the ones that I included
with the kmacro-tests.el patch in bug#24939.
I also rewrote one test from files-tests.el as an example of usage and
included it with the patch. It shows how the macros can help make the
logic of a test clearer by removing the clutter of extra variables used
to keep track of the arguments passed in function calls made by the code
under test. For more examples, see the kmacro-tests.el patch in
bug#24939.
Let me know if you see ways to make this code better, or if there's any
part of adding functionality to Emacs that I've missed here.
[-- Attachment #2: 0001-Add-should-call-should-not-call-and-their-tests.patch --]
[-- Type: text/plain, Size: 24239 bytes --]
From 8c24022658738c3e32f94d1033caf7df36142a46 Mon Sep 17 00:00:00 2001
From: gazally <gazally@users.noreply.github.com>
Date: Sun, 13 Nov 2016 10:50:23 -0800
Subject: [PATCH] Add should-call, should-not-call, and their tests
* lisp/emacs-lisp/ert.el (should-call, should-not-call): New macros.
* doc/misc/ert.texi
(How to Write Tests): Document should-call and should-not-call.
(Mocks and Stubs): Mention should-call, and delete old Emacs wiki
reference.
* test/lisp/emacs-lisp/ert-tests.el
(ert-test-verify-no-advice, ert-test-should-call-fails-test-on-no-call)
(ert-test-should-call-fails-test-when-call-count-incorrect)
(ert-test-should-call-collects-arg-list)
(ert-test-should-call-check-args-with-failure)
(ert-test-should-not-call-fails-test-when-function-called)
(ert-test-should-call-cleans-up-after-failure-in-user-advice):
Tests for should-call and should-not-call.
(ert-test-testfunc1, ert-test-testfunc2, ert-test-testfunc3)
(ert-test-verify-no-advice): Helper functions for should-call and
should-not-call tests.
* test/lisp/files-tests.el
(files-test--save-buffers-kill-emacs--confirm-kill-processes):
Use should-call and should-not-call instead of cl-letf.
---
doc/misc/ert.texi | 81 +++++++++++++++++++--
etc/NEWS | 5 ++
lisp/emacs-lisp/ert.el | 147 +++++++++++++++++++++++++++++++++++---
test/lisp/emacs-lisp/ert-tests.el | 128 +++++++++++++++++++++++++++++++++
test/lisp/files-tests.el | 31 ++++----
5 files changed, 358 insertions(+), 34 deletions(-)
diff --git a/doc/misc/ert.texi b/doc/misc/ert.texi
index 144dfd9..f1ff265 100644
--- a/doc/misc/ert.texi
+++ b/doc/misc/ert.texi
@@ -73,6 +73,7 @@ Top
How to Write Tests
* The @code{should} Macro:: A powerful way to express assertions.
+* The @code{should-call} Macro:: Testing interactions with other code.
* Expected Failures:: Tests for known bugs.
* Tests and Their Environment:: Don't depend on customizations; no side effects.
* Useful Techniques:: Some examples.
@@ -353,6 +354,7 @@ How to Write Tests
@menu
* The @code{should} Macro:: A powerful way to express assertions.
+* The @code{should-call} Macro:: Testing interactions with other code.
* Expected Failures:: Tests for known bugs.
* Tests and Their Environment:: Don't depend on customizations; no side effects.
* Useful Techniques:: Some examples.
@@ -421,6 +423,76 @@ The @code{should} Macro
@xref{Understanding Explanations}, for more details on what
@code{should} reports.
+@node The @code{should-call} Macro
+@section The @code{should-call} Macro
+
+Sometimes when writing tests for code that is part of a complicated
+system, it is necessary to test that calls to an underlying interface
+are made correctly. Sometimes such checking can be done while the
+underlying code runs normally and sometimes it is better to prevent
+that code from running, for example if it makes changes to Emacs's
+global state.
+
+Emacs Lisp's advice mechanism is ideal for this sort of work, and the
+@code{should-call} macro can add and remove advice on named functions,
+while making sure that any added advice is cleaned up if a test fails.
+An example use of @code{should-call}:
+
+@lisp
+(ert-deftest correct-usage ()
+ (should-call ((useful-function :once
+ :before (lambda (arg)
+ (should (equal arg "test"))))
+ (expensive-function :times 2
+ :override (lambda (arg)
+ (should (integerp arg)))))
+ (function-to-test "test" 2)))
+@end lisp
+
+This test will pass if @code{function-to-test} calls
+@code{useful-function} once with @code{"test"}, and
+@code{expensive-function} twice with an integer argument each
+time. @code{useful-function} will be called but
+@code{expensive-function} will not be. The order in which the calls
+happen does not matter in this example.
+
+Like @code{let}, @code{should-call} takes a list of bindings and a
+body of code to execute. Each binding begins with a symbol already
+bound to a function, and is followed by a description of the check to
+make on the number of times the function is called, which can be
+@code{:once}, @code{:times} followed by a number, or
+@code{:check-args-with} followed by a function. The last part of each
+binding is optional, and provides advice to attach to the function
+during the execution of your test code. The advice is described by a
+keyword calling method and function exactly as for
+@code{advice-add}. Here is an example where advice is not given and
+@code{:check-args-with} is used:
+
+@lisp
+(ert-deftest process-data-total ()
+ (should-call ((process-data :check-args-with
+ (lambda (arglist)
+ (eql 500
+ (apply #'+
+ (mapcar #'car arglist))))))
+ (function-to-test (make-list 500 ?x))))
+@end lisp
+
+The function form following @code{:check-args-with} is passed a list
+of all the argument lists given to the advised function (in reverse
+order). The test will pass or fail depending on the return value of
+the argument check function. So the test above does not set any
+expectation of how many times @code{function-to-test} calls
+@code{process-data}, just that the sum of all the first arguments in
+all the calls is the expected value.
+
+In addition to @code{should-call}, ERT provides
+@code{should-not-call}, which when given either a single named
+function or a list of them, and a body of code to execute, will cause
+the test to fail if any are called.
+
+@xref{Advising Functions,,, elisp, GNU Emacs Lisp Reference Manual},
+for more information on ways in which advice may be added to a function.
@node Expected Failures
@section Expected Failures
@@ -813,10 +885,11 @@ Mocks and Stubs
@url{http://en.wikipedia.org/wiki/Mock_object} for an explanation of
the corresponding concepts in object-oriented languages.
-ERT does not have built-in support for mocks or stubs. The package
-@code{el-mock} (see @url{http://www.emacswiki.org/emacs/el-mock.el})
-offers mocks for Emacs Lisp and can be used in conjunction with ERT.
-
+ERT does not have built-in support for mocks or stubs. A global
+function definition can be redefined for the duration of a test using
+@code{cl-letf}. Emacs Lisp's advice mechanism can be used to attach
+additional functionality to a function in a variety of ways, and ERT's
+@code{should-call} macro can attach temporary advice during a test.
@node Fixtures and Test Suites
@section Fixtures and Test Suites
diff --git a/etc/NEWS b/etc/NEWS
index fe76af5..4619cd2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -686,6 +686,11 @@ that does not exist.
operating recursively and when some other process deletes the directory
or its files before 'delete-directory' gets to them.
+** The new macro 'should-call' adds advice to one or more global
+functions for the duration of a test, and requires that the functions
+be called by the test. The new macro 'should-not-call' uses advice to
+do the opposite.
+
** Changes in Frame- and Window- Handling
+++
diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index 0308c9c..09f882a 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -34,17 +34,21 @@
;; `ert-run-tests-batch-and-exit' for non-interactive use.
;;
;; The body of `ert-deftest' forms resembles a function body, but the
-;; additional operators `should', `should-not', `should-error' and
-;; `skip-unless' are available. `should' is similar to cl's `assert',
-;; but signals a different error when its condition is violated that
-;; is caught and processed by ERT. In addition, it analyzes its
-;; argument form and records information that helps debugging
-;; (`assert' tries to do something similar when its second argument
-;; SHOW-ARGS is true, but `should' is more sophisticated). For
-;; information on `should-not' and `should-error', see their
-;; docstrings. `skip-unless' skips the test immediately without
-;; processing further, this is useful for checking the test
-;; environment (like availability of features, external binaries, etc).
+;; additional operators `should', `should-not', `should-error',
+;; `should-call' and `should-not-call' `skip-unless' are available.
+
+;; `should' is similar to cl's `assert', but signals a different error
+;; when its condition is violated that is caught and processed by ERT.
+;; In addition, it analyzes its argument form and records information
+;; that helps debugging (`assert' tries to do something similar when
+;; its second argument SHOW-ARGS is true, but `should' is more
+;; sophisticated). For information on `should-not', `should-error',
+;; `should-call' and `should-not-call', see their docstrings.
+;;
+;; `skip-unless' skips the test immediately without
+;; processing further. This is useful for checking the test
+;; environment (like availability of features, external binaries,
+;; etc).
;;
;; See ERT's info manual as well as the docstrings for more details.
;; To compile the manual, run `makeinfo ert.texinfo' in the ERT
@@ -367,6 +371,127 @@ ert--expand-should
`(unless (not ,inner-form)
(ert-fail ,form-description-form)))))
+(defmacro should-call (defs &rest body)
+ "Verify that the function(s) in DEFS are called by BODY.
+
+DEFS should be a list containing elements of the form:
+ (FUNC ARGCHECK WHERE FUNCTION)
+
+where FUNC is a symbol, ARGCHECK is either :once, :times followed
+by a value, or :check-args-with followed by a function value.
+WHERE and FUNCTION are optional and have the same meaning as in
+`advice-add'.
+
+Temporarily add advice to each FUNC in DEFS, including advice
+which records the arguments passed to FUNC (by reference not
+copy, relevant for destructive functions), execute BODY, and then
+depending on the ARGCHECK forms, verify that FUNC was either
+called once, the specified number of times, or that the function
+following :check-args-with returns a non-nil value when passed a
+list of all the arguments passed to FUNC (which will be in
+reverse order). If any of the checks fail, abort the current
+test as failed."
+ (declare (debug ((&rest [fboundp
+ (&or ":once"
+ [":times" form]
+ [":check-args-with" function-form])
+ &optional keywordp function-form])
+ body))
+ (indent 1))
+ (ert--expand-should-or-should-not-call defs body))
+
+(defmacro should-not-call (func-or-funcs &rest body)
+ "Verify that FUNC-OR-FUNCS are not called by BODY.
+FUNC-OR-FUNCS can either be a single function or a list of them.
+Add advice to them that will cause the test to fail if any are
+called during the execution of BODY."
+ (declare (debug (&or [(&rest fboundp) body]
+ [fboundp body]))
+ (indent 1))
+ (let* ((funcs (if (consp func-or-funcs)
+ func-or-funcs
+ (list func-or-funcs)))
+ (defs (mapcar (lambda (f) (list f :not)) funcs)))
+ (ert--expand-should-or-should-not-call defs body)))
+
+(defun ert--expand-should-or-should-not-call (defs body)
+ "Helper function for should-call and should-not-call.
+DEFS and BODY are the same as for should-call, except that one additional
+ARGCHECK keyword is allowed, :not, for use by should-not-call."
+ (if (null defs)
+ `(progn ,@body)
+ (let* ((def (car defs))
+ (func (car def))
+ (g-arglist (cl-gensym "args-list-"))
+ (g-argrec (cl-gensym "args-rec-"))
+ (g-advice (cl-gensym "should-call-advice-"))
+ (g-call-count (cl-gensym "call-count-"))
+ (argcheck-type (nth 1 def))
+ (check-val (unless (memq argcheck-type '(:once :not)) (nth 2 def)))
+ (form-description-form `(should-call ("..." ,def "...") ,@body))
+ (advice-given (> (length def) 3))
+ (advice-keyword (and advice-given (car (last def 2))))
+ (advice-function (and advice-given (car (last def)))))
+
+ (when (eq argcheck-type :once)
+ (setq argcheck-type :times)
+ (setq check-val 1))
+ (when (eq argcheck-type :not)
+ (setq form-description-form
+ `(should-not-call ("..." ,func "...") ,@body))
+ (setq advice-given t)
+ (setq advice-keyword :override)
+ (setq advice-function `(lambda (&rest _args)
+ (ert-fail (list
+ ',form-description-form
+ :fail-reason
+ (format "%s was called" ',func))))))
+ ;; Add two pieces of advice to the function: the one provided in
+ ;; the definitions list, and another to record the arguments.
+ `(let* (,g-arglist
+ (,g-argrec (lambda (&rest args)
+ (push args ,g-arglist)))
+ ,@(when advice-given
+ `((,g-advice ,advice-function)))) ; only evaluate this once
+ (advice-add ',func :before ,g-argrec '((depth . -100)))
+ (unwind-protect
+ (progn
+ ,@(when advice-given
+ `((advice-add ',func ,advice-keyword ,g-advice
+ '((depth . -99)))))
+ (unwind-protect
+ ,(ert--expand-should-or-should-not-call (cdr defs) body)
+ ,@(when advice-given
+ `((advice-remove ',func ,g-advice)))))
+ (advice-remove ',func ,g-argrec))
+ ;; Generate the after-execution argument list check.
+ ,(cond
+ ((eq argcheck-type :times)
+ `(let ((,g-call-count (length ,g-arglist)))
+ (unless (eql ,g-call-count ,check-val)
+ (ert-fail (list
+ ',form-description-form
+ :fail-reason
+ (format (cond
+ ((zerop ,check-val)
+ "%s was called")
+ ((zerop ,g-call-count)
+ "%s was not called")
+ (t "%s was called %s time%s"))
+ ',func ,g-call-count
+ (if (eql 1 ,g-call-count)
+ "" "s")))))))
+ ((eq argcheck-type :check-args-with)
+ `(unless (funcall ,check-val ,g-arglist)
+ (ert-fail (list
+ ',form-description-form
+ :condition
+ (list 'apply ',check-val ,g-arglist)
+ :fail-reason
+ ":check-args-with null result")))))
+ (ert--signal-should-execution ',form-description-form)))))
+
+
(defun ert--should-error-handle-error (form-description-fn
condition type exclude-subtypes)
"Helper function for `should-error'.
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 5d36755..bec6962 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -294,6 +294,134 @@ ert-self-test-and-exit
"the error signaled was a subtype of the expected type")))))
))
+;; Some named functions for should-call testing
+
+(defvar ert-test-testfunc-counters [0 0 0])
+(defun ert-test-testfunc1 (arg)
+ (incf (aref ert-test-testfunc-counters 0)))
+
+(defun ert-test-testfunc2 (arg)
+ (incf (aref ert-test-testfunc-counters 1)))
+
+(defun ert-test-testfunc3 (arg)
+ (incf (aref ert-test-testfunc-counters 2)))
+
+(defun ert-test-verify-no-advice (sym)
+ "Verify that SYM has no advice attached to it."
+ (let (advice)
+ (advice-mapc (lambda (&rest args) (push args advice)) sym)
+ (should-not advice)))
+
+(ert-deftest ert-test-should-call-fails-test-on-no-call ()
+ "`should-call' fails test if function not called."
+ (let ((funcs '(ert-test-testfunc1 ert-test-testfunc2 ert-test-testfunc3))
+ (ert-test-testfunc-counters (make-vector 3 0)))
+ (dolist (omitted funcs)
+ (let* ((funcs-to-call (remq omitted funcs))
+ (test (make-ert-test :body (lambda ()
+ (should-call ((ert-test-testfunc1 :once)
+ (ert-test-testfunc2 :once)
+ (ert-test-testfunc3 :once))
+ (dolist (f funcs-to-call)
+ (funcall f nil))))))
+ (result (ert-run-test test)))
+ (should (ert-test-failed-p result))
+ (pcase (ert-test-result-with-condition-condition result)
+ (`(ert-test-failed (,_form :fail-reason ,msg))
+ (should (string= (format "%s was not called" omitted)
+ msg)))
+ (_
+ (should-not (or result t))))))
+ ;; Make sure all advice was removed
+ (dolist (f funcs)
+ (ert-test-verify-no-advice f))
+ ;; Make sure test functions got called
+ (should (equal [2 2 2] ert-test-testfunc-counters))))
+
+(ert-deftest ert-test-should-call-fails-test-when-call-count-incorrect ()
+ "`should-call' fails test if function not called the correct number of times."
+ (let* ((ert-test-testfunc-counters (make-vector 3 0))
+ (test (make-ert-test :body (lambda ()
+ (should-call ((ert-test-testfunc1 :times 2)
+ (ert-test-testfunc2 :times 2))
+ (ert-test-testfunc1 nil)
+ (ert-test-testfunc2 nil)
+ (ert-test-testfunc2 nil)))))
+ (result (ert-run-test test)))
+ (should (ert-test-failed-p result))
+ (pcase (ert-test-result-with-condition-condition result)
+ (`(ert-test-failed (,_form :fail-reason ,msg))
+ (should (string= "ert-test-testfunc1 was called 1 time" msg)))
+ (_ (should-not (or result t))))
+ (should (equal [1 2 0] ert-test-testfunc-counters)))
+ (ert-test-verify-no-advice 'ert-test-testfunc1))
+
+(ert-deftest ert-test-should-call-collects-arg-list ()
+ "`should-call' collects function arguments if :check-args-with is used."
+ (let ((ert-test-testfunc-counters (make-vector 3 0)))
+ (should-call ((ert-test-testfunc1 :check-args-with
+ (lambda (arglist)
+ (equal arglist '((4) (3) (2) (1) (0))))))
+ (dotimes (n 5)
+ (ert-test-testfunc1 n)))
+ (should (equal [5 0 0] ert-test-testfunc-counters)))
+ (ert-test-verify-no-advice 'ert-test-testfunc1))
+
+(ert-deftest ert-test-should-call-check-args-with-failure ()
+ "`should-call' causes test to fail if :check-args-with lambda returns nil."
+ (let* ((ert-test-testfunc-counters (make-vector 3 0))
+ (test (make-ert-test :body (lambda ()
+ (should-call ((ert-test-testfunc1
+ :check-args-with
+ 'ignore))
+ (ert-test-testfunc1 42)))))
+ (result (ert-run-test test)))
+ (should (ert-test-failed-p result))
+ (should (equal (ert-test-result-with-condition-condition result)
+ '(ert-test-failed ((should-call ("..." (ert-test-testfunc1
+ :check-args-with
+ 'ignore)
+ "...")
+ (ert-test-testfunc1 42))
+ :condition (apply (quote ignore) ((42)))
+ :fail-reason
+ ":check-args-with null result"))))
+ (should (equal [1 0 0] ert-test-testfunc-counters)))
+ (ert-test-verify-no-advice 'ert-test-testfunc1))
+
+(ert-deftest ert-test-should-not-call-fails-test-when-function-called ()
+ "`should-not-call' causes test to fail if listed function is called."
+ (let* ((ert-test-testfunc-counters (make-vector 3 0))
+ (test (make-ert-test :body (lambda ()
+ (should-not-call (ert-test-testfunc1
+ ert-test-testfunc2)
+ (ert-test-testfunc1 nil)))))
+ (result (ert-run-test test)))
+ (should (ert-test-failed-p result))
+ (should (equal (ert-test-result-with-condition-condition result)
+ '(ert-test-failed ((should-not-call ("..."
+ ert-test-testfunc1
+ "...")
+ (ert-test-testfunc1 nil))
+ :fail-reason
+ "ert-test-testfunc1 was called"))))
+ (should (equal [0 0 0] ert-test-testfunc-counters)))
+ (ert-test-verify-no-advice 'ert-test-testfunc1))
+
+(ert-deftest ert-test-should-call-cleans-up-after-failure-in-user-advice ()
+ "`should-call' removes advice after an error in supplied advice function."
+ (let ((ert-test-testfunc-counters (make-vector 3 0)))
+ (should-error
+ (should-call ((ert-test-testfunc1 :once :override #'ignore)
+ (ert-test-testfunc2 :once :around (lambda (func arg)
+ (signal 'arith-error nil))))
+ (ert-test-testfunc1 nil)
+ (ert-test-testfunc2 nil)))
+ (should (equal [0 0 0] ert-test-testfunc-counters)))
+ (ert-test-verify-no-advice 'ert-test-testfunc1)
+ (ert-test-verify-no-advice 'ert-test-testfunc2))
+
+
(ert-deftest ert-test-skip-unless ()
;; Don't skip.
(let ((test (make-ert-test :body (lambda () (skip-unless t)))))
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 80d5e5b..753ea78 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -200,25 +200,18 @@ files-test-bug-18141-file
(ert-deftest files-test--save-buffers-kill-emacs--confirm-kill-processes ()
"Test that `save-buffers-kill-emacs' honors
`confirm-kill-processes'."
- (cl-letf* ((yes-or-no-p-prompts nil)
- ((symbol-function #'yes-or-no-p)
- (lambda (prompt)
- (push prompt yes-or-no-p-prompts)
- nil))
- (kill-emacs-args nil)
- ((symbol-function #'kill-emacs)
- (lambda (&optional arg) (push arg kill-emacs-args)))
- (process
- (make-process
- :name "sleep"
- :command (list
- (expand-file-name invocation-name invocation-directory)
- "-batch" "-Q" "-eval" "(sleep-for 1000)")))
- (confirm-kill-processes nil))
- (save-buffers-kill-emacs)
- (kill-process process)
- (should-not yes-or-no-p-prompts)
- (should (equal kill-emacs-args '(nil)))))
+ (should-call ((kill-emacs :once :override (lambda (&optional arg)
+ (should-not arg))))
+ (should-not-call yes-or-no-p
+ (let ((process
+ (make-process
+ :name "sleep"
+ :command (list
+ (expand-file-name invocation-name invocation-directory)
+ "-batch" "-Q" "-eval" "(sleep-for 1000)")))
+ (confirm-kill-processes nil))
+ (save-buffers-kill-emacs)
+ (kill-process process)))))
(provide 'files-tests)
;;; files-tests.el ends here
--
2.10.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
2016-11-13 22:22 bug#24940: [PATCH] Add should-call, should-not-call, and their tests Gemini Lasswell
@ 2016-11-14 15:50 ` Eli Zaretskii
2016-11-15 5:52 ` Gemini Lasswell
2016-11-14 16:18 ` Phillip Lord
1 sibling, 1 reply; 8+ messages in thread
From: Eli Zaretskii @ 2016-11-14 15:50 UTC (permalink / raw)
To: Gemini Lasswell; +Cc: 24940
> From: Gemini Lasswell <gazally@runbox.com>
> Date: Sun, 13 Nov 2016 14:22:08 -0800
>
> In the process of writing tests for kmacro.el, I wrote two macros,
> should-call and should-not-call, that create context by temporarily
> adding advice to named functions. They allow a test writer to express
> expectations of how functions are used, to mock up responses of those
> functions to the code under test, and to prevent functions from running
> which might modify the global state of Emacs in an undesirable way
> during a test.
I don't think our test suite should in general test whether some
functions are or aren't called during execution of the
command/function under test. That's because we are not interested in
the fine details of the implementation of the features being tested,
we are interested in whether it does what it's supposed to do. "What
it's supposed to do" means we should test the return values and the
side effects, a.k.a. "the behavior", and not how the code works
internally.
Using your proposed approach (amply represented in the tests you
submitted for kmacro.el) has several disadvantages, beyond the above
fundamental issue:
. it makes it impossible to write tests before the features are
implemented (TDD), since the details of the implementation,
i.e. who calls what and when, are not yet known
. it tends to produce tests that are much more complex than they have
to be, due to the need to disable or override internal code
workings, so the tested code does what you want, without getting in
your way
. by messing with so much of the internal workings, there's a high
risk that the tested code is significantly different from what runs
in a real Emacs session, which invalidates the test results to some
degree -- you are no longer testing the original code
. the produced tests are extremely fragile, and will need a lot of
maintenance whenever something changes in the tested features'
implementation, or in the infrastructure they use, or in how
certain exceptional situations are handled by that infrastructure
I consider the should-call and should-not-call tests appropriate only
when the behavior of the function is explicitly specified to make
these calls. About the only applicable use case is when a function is
specified to prompt the user under some circumstances and not the
others. And even then it's fragile, because "prompt the user" is not
equal to "call yes-or-no-p", even though the current implementation
might do the latter. A better way, though perhaps much harder, would
be to test something much more basic that is common to _any_ prompt,
like look in the echo-area or the minibuffer buffers for suitable
text, or maybe even C-level infrastructure that catches the situation
where Emacs waits for user input.
If we can agree that the above use cases are the only ones where such
macros should be used, then these macros should be much leaner than
what you propose, because there should be no need to provide many of
the features in your proposal, like testing the arguments and the
return value, or the number of times the subroutines are called.
> I also rewrote one test from files-tests.el as an example of usage and
> included it with the patch. It shows how the macros can help make the
> logic of a test clearer by removing the clutter of extra variables used
> to keep track of the arguments passed in function calls made by the code
> under test. For more examples, see the kmacro-tests.el patch in
> bug#24939.
I think it's no coincidence you did that with a test whose purpose is
to make sure the user is not prompted under certain conditions. I
think these are about the only valid use cases for such functionality.
However, note that your kmacro.el tests employ these macros much more
extensively, mostly where we don't care about which functions are
called, but about the produced effect. When there's an expected
effect, we should test that effect directly, instead of relying on
certain functions being called, and assuming they will produce the
desired effect.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
2016-11-13 22:22 bug#24940: [PATCH] Add should-call, should-not-call, and their tests Gemini Lasswell
2016-11-14 15:50 ` Eli Zaretskii
@ 2016-11-14 16:18 ` Phillip Lord
2016-11-15 6:12 ` Gemini Lasswell
2019-06-24 23:43 ` Lars Ingebrigtsen
1 sibling, 2 replies; 8+ messages in thread
From: Phillip Lord @ 2016-11-14 16:18 UTC (permalink / raw)
To: Gemini Lasswell; +Cc: 24940
That's interesting. I'm probably going to add my own package, assess, to
ELPA soon, and then to core, which has some similar functionality.
https://github.com/phillord/assess
Out of curiosity, would assess have fulfilled your use case also?
Gemini Lasswell <gazally@runbox.com> writes:
> In the process of writing tests for kmacro.el, I wrote two macros,
> should-call and should-not-call, that create context by temporarily
> adding advice to named functions. They allow a test writer to express
> expectations of how functions are used, to mock up responses of those
> functions to the code under test, and to prevent functions from running
> which might modify the global state of Emacs in an undesirable way
> during a test.
>
> I think that these macros would be useful additions to ERT. Here is a
> patch containing versions of the macros which are integrated into ERT
> and which provide better failure reporting than the ones that I included
> with the kmacro-tests.el patch in bug#24939.
>
> I also rewrote one test from files-tests.el as an example of usage and
> included it with the patch. It shows how the macros can help make the
> logic of a test clearer by removing the clutter of extra variables used
> to keep track of the arguments passed in function calls made by the code
> under test. For more examples, see the kmacro-tests.el patch in
> bug#24939.
>
> Let me know if you see ways to make this code better, or if there's any
> part of adding functionality to Emacs that I've missed here.
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
2016-11-14 15:50 ` Eli Zaretskii
@ 2016-11-15 5:52 ` Gemini Lasswell
2016-11-15 15:24 ` Eli Zaretskii
2020-08-11 13:23 ` Lars Ingebrigtsen
0 siblings, 2 replies; 8+ messages in thread
From: Gemini Lasswell @ 2016-11-15 5:52 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 24940
Eli Zaretskii <eliz@gnu.org> writes:
> I don't think our test suite should in general test whether some
> functions are or aren't called during execution of the
> command/function under test. That's because we are not interested in
> the fine details of the implementation of the features being tested,
> we are interested in whether it does what it's supposed to do. "What
> it's supposed to do" means we should test the return values and the
> side effects, a.k.a. "the behavior", and not how the code works
> internally.
I'll modify the tests I've written to focus on "the behavior".
> might do the latter. A better way, though perhaps much harder, would
> be to test something much more basic that is common to _any_ prompt,
> like look in the echo-area or the minibuffer buffers for suitable
> text, or maybe even C-level infrastructure that catches the situation
> where Emacs waits for user input.
I agree that infrastructure to allow a test to set up input to be
supplied to the next prompt and to read the prompt message would be a
better solution.
Keyboard macros work to simulate user input for any test code that isn't
trying to test kmacro-step-edit-macro. If there was a hook that the C
code could run just prior to taking input from a keyboard macro, tests
could use it to check that the correct prompt appears at the correct
point in the macro.
Such a hook would also be helpful in the attempt to fix
kmacro-step-edit-macro's bugs. Mostly where it messes up now is when
several input events happen during one command, and a pre-input hook
would allow it to inspect keyboard macro execution event by event rather
than command by command.
The remaining problem that I see is that lots of code in lots of places
suppresses messages depending on called-interactively-p,
executing-kbd-macro and/or noninteractive, and although that doesn't
just affect tests that use keyboard macros, it does make it hard to test
how Emacs behaves interactively.
> If we can agree that the above use cases are the only ones where such
> macros should be used, then these macros should be much leaner than
> what you propose, because there should be no need to provide many of
> the features in your proposal, like testing the arguments and the
> return value, or the number of times the subroutines are called.
The leanest version I can think of would be:
(with-added-advice BINDINGS
BODY)
where BINDINGS is a list of argument lists for advice-add. It would call
advice-add once for each element of BINDINGS, execute BODY, and then
remove all the added advice. Then it looks like something that belongs
in nadvice.el instead of ert.el.
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
2016-11-14 16:18 ` Phillip Lord
@ 2016-11-15 6:12 ` Gemini Lasswell
2019-06-24 23:43 ` Lars Ingebrigtsen
1 sibling, 0 replies; 8+ messages in thread
From: Gemini Lasswell @ 2016-11-15 6:12 UTC (permalink / raw)
To: Phillip Lord; +Cc: 24940
phillip.lord@russet.org.uk (Phillip Lord) writes:
> That's interesting. I'm probably going to add my own package, assess, to
> ELPA soon, and then to core, which has some similar functionality.
>
> https://github.com/phillord/assess
>
> Out of curiosity, would assess have fulfilled your use case also?
Yes it would have.
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
2016-11-15 5:52 ` Gemini Lasswell
@ 2016-11-15 15:24 ` Eli Zaretskii
2020-08-11 13:23 ` Lars Ingebrigtsen
1 sibling, 0 replies; 8+ messages in thread
From: Eli Zaretskii @ 2016-11-15 15:24 UTC (permalink / raw)
To: Gemini Lasswell; +Cc: 24940
> From: Gemini Lasswell <gazally@runbox.com>
> Cc: 24940@debbugs.gnu.org
> Date: Mon, 14 Nov 2016 21:52:23 -0800
>
> I'll modify the tests I've written to focus on "the behavior".
Thank you.
> > might do the latter. A better way, though perhaps much harder, would
> > be to test something much more basic that is common to _any_ prompt,
> > like look in the echo-area or the minibuffer buffers for suitable
> > text, or maybe even C-level infrastructure that catches the situation
> > where Emacs waits for user input.
>
> I agree that infrastructure to allow a test to set up input to be
> supplied to the next prompt and to read the prompt message would be a
> better solution.
You are talking about mocking user input, whereas I was talking about
a feature that could tell whether Emacs asked for user input in any
way, even if the user herself provided that input.
Of course, mocking input is also an important part of a test suite.
> The remaining problem that I see is that lots of code in lots of places
> suppresses messages depending on called-interactively-p,
> executing-kbd-macro and/or noninteractive, and although that doesn't
> just affect tests that use keyboard macros, it does make it hard to test
> how Emacs behaves interactively.
We could have a separate knob for that, to be used by the test suite.
> > If we can agree that the above use cases are the only ones where such
> > macros should be used, then these macros should be much leaner than
> > what you propose, because there should be no need to provide many of
> > the features in your proposal, like testing the arguments and the
> > return value, or the number of times the subroutines are called.
>
> The leanest version I can think of would be:
>
> (with-added-advice BINDINGS
> BODY)
>
> where BINDINGS is a list of argument lists for advice-add. It would call
> advice-add once for each element of BINDINGS, execute BODY, and then
> remove all the added advice.
No, I meant a macro that would return non-nil if some FUNCTION was
called during execution of that macro's body.
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
2016-11-14 16:18 ` Phillip Lord
2016-11-15 6:12 ` Gemini Lasswell
@ 2019-06-24 23:43 ` Lars Ingebrigtsen
1 sibling, 0 replies; 8+ messages in thread
From: Lars Ingebrigtsen @ 2019-06-24 23:43 UTC (permalink / raw)
To: Phillip Lord; +Cc: Gemini Lasswell, 24940
phillip.lord@russet.org.uk (Phillip Lord) writes:
> That's interesting. I'm probably going to add my own package, assess, to
> ELPA soon, and then to core, which has some similar functionality.
>
> https://github.com/phillord/assess
It looks like it wasn't added to ELPA or core?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24940: [PATCH] Add should-call, should-not-call, and their tests
2016-11-15 5:52 ` Gemini Lasswell
2016-11-15 15:24 ` Eli Zaretskii
@ 2020-08-11 13:23 ` Lars Ingebrigtsen
1 sibling, 0 replies; 8+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-11 13:23 UTC (permalink / raw)
To: Gemini Lasswell; +Cc: 24940
Gemini Lasswell <gazally@runbox.com> writes:
>> I don't think our test suite should in general test whether some
>> functions are or aren't called during execution of the
>> command/function under test. That's because we are not interested in
>> the fine details of the implementation of the features being tested,
>> we are interested in whether it does what it's supposed to do. "What
>> it's supposed to do" means we should test the return values and the
>> side effects, a.k.a. "the behavior", and not how the code works
>> internally.
>
> I'll modify the tests I've written to focus on "the behavior".
Gemini, this was apparently never done? Did you abandon this patch in
favour of Phillip's assess package? (Which I see is in Melpa now.)
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-11 13:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-13 22:22 bug#24940: [PATCH] Add should-call, should-not-call, and their tests Gemini Lasswell
2016-11-14 15:50 ` Eli Zaretskii
2016-11-15 5:52 ` Gemini Lasswell
2016-11-15 15:24 ` Eli Zaretskii
2020-08-11 13:23 ` Lars Ingebrigtsen
2016-11-14 16:18 ` Phillip Lord
2016-11-15 6:12 ` Gemini Lasswell
2019-06-24 23:43 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).