[PATCH Elpa: streams.el] Add systematic tests against bogus element generation

[Michael Heerdegen]

Hi,

we very often made the error of implementing functions like `seq-map'
for streams so that they unnecessarily generated elements from input
streams (and as you'll see, we are still doing this in some cases...)

But we currently only have one test for `seq-map'.  This patch
implements a new class of tests for all functions where this problem can
potentially occur.  In addition, because this problem is so common (and
it's not so obvious that even the use of `stream-empty-p' is
problematic), I also added a note to the header.

The tests reveal two errors we had not discovered yet.  I'll fix these
in one of the next commits.

Ok, and here is the proposed patch.


Regards,

Michael.


>From 1771087232cb7e8356a5a3d1a338732da12bdd7a Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <address@hidden>
Date: Fri, 16 Sep 2016 01:20:45 +0200
Subject: [PATCH] Add systematic tests against bogus element generation

Also add a note about this problem in the header and how to avoid it.
---
 packages/stream/stream.el             | 13 +++++++++++-
 packages/stream/tests/stream-tests.el | 38 +++++++++++++++++++++++++++++------
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/packages/stream/stream.el b/packages/stream/stream.el
index 9954fc8..ef19918 100644
--- a/packages/stream/stream.el
+++ b/packages/stream/stream.el
@@ -4,7 +4,7 @@
 
 ;; Author: Nicolas Petton <address@hidden>
 ;; Keywords: stream, laziness, sequences
-;; Version: 2.2.1
+;; Version: 2.2.2
 ;; Package-Requires: ((emacs "25"))
 ;; Package: stream
 
@@ -49,6 +49,17 @@
 ;; (defun fib (a b)
 ;;  (stream-cons a (fib b (+ a b))))
 ;; (fib 0 1)
+;;
+;; A note for developers: Please make sure to implement functions that
+;; process streams (build new streams out of given streams) in a way
+;; that no new elements in any argument stream are generated.  This is
+;; most likely an error since it changes the argument stream.  For
+;; example, a common error is to call `stream-empty-p' on an input
+;; stream and build the stream to return depending on the result.
+;; Instead, delay such tests until elements are requested from the
+;; resulting stream.  A way to achieve this is to wrap such tests into
+;; `stream-make' or `stream-delay'.  See the implementations of
+;; `stream-append' or `seq-drop-while' for example.
 
 ;;; Code:
 
diff --git a/packages/stream/tests/stream-tests.el 
b/packages/stream/tests/stream-tests.el
index 31c3530..e79c3ef 100644
--- a/packages/stream/tests/stream-tests.el
+++ b/packages/stream/tests/stream-tests.el
@@ -234,12 +234,6 @@
   (should (= (seq-length (seq-subseq (stream-range 2 10) 1 3)) 2))
   (should (= (seq-elt (seq-subseq (stream-range 2 10) 1 3) 1) 4)))
 
-(ert-deftest stream-seq-map-should-not-consume-stream-elements ()
-  (let* (consumed
-         (stream (stream-cons (setq consumed t) (stream-empty))))
-    (seq-map #'identity stream)
-    (should-not consumed)))
-
 (ert-deftest stream-pop-test ()
   (let* ((str (stream '(1 2 3)))
          (first (stream-pop str))
@@ -271,5 +265,37 @@
                                  (stream (list 5 6 7 8 9))))))
                  (list 1 2 3 4 5 6 7 8 9))))
 
+;; Tests whether calling stream processing functions ("transducers")
+;; doesn't generate elements from argument streams
+
+(defvar this-delayed-stream-function nil)
+
+(defun make-delayed-test-stream ()
+  (stream-make
+   (cons (prog1 1 (error "`%s' not completely delayed" 
this-delayed-stream-function))
+         (make-delayed-test-stream))))
+
+(defmacro deftest-for-delayed-evaluation (call)
+  (let ((function (car call)))
+    `(ert-deftest ,(intern (concat (symbol-name function) "-delayed-test")) ()
+       (let ((this-delayed-stream-function ',function))
+         (should (prog1 t ,call))))))
+
+(deftest-for-delayed-evaluation (streamp        (make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (seqp           (make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (stream-append  (make-delayed-test-stream) 
(make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (seq-take (make-delayed-test-stream) 2))
+(deftest-for-delayed-evaluation (seq-drop (make-delayed-test-stream) 2))
+(deftest-for-delayed-evaluation (seq-take-while #'numberp 
(make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (seq-take-until #'numberp 
(make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (seq-map #'identity 
(make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (seq-filter #'cl-evenp 
(make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (stream-delay (make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (seq-copy (make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (seq-subseq (make-delayed-test-stream) 2))
+(deftest-for-delayed-evaluation (stream-scan #'* 1 (make-delayed-test-stream)))
+(deftest-for-delayed-evaluation (stream-concatenate (stream (list 
(make-delayed-test-stream)
+                                                                  
(make-delayed-test-stream)))))
+
 (provide 'stream-tests)
 ;;; stream-tests.el ends here
-- 
2.9.3