[PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument

[PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument

[Sebastian Miele]

* testing/lisp/test-ob-emacs-lisp.el
  (test-ob-emacs-lisp-dynamic-lexical-text,
  test-ob-emacs-lisp-dynamic-lexical-expr,
  ob-emacs-lisp/dynamic-lexical-execute,
  ob-emacs-lisp/dynamic-lexical-edit): Add tests that check the
  correct handling of the :lexical header argument when executing
  source blocks and when creating editing buffers for source blocks.
---
 testing/lisp/test-ob-emacs-lisp.el | 86 ++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/testing/lisp/test-ob-emacs-lisp.el b/testing/lisp/test-ob-emacs-lisp.el
index 078cad988..a48f0c7dd 100644
--- a/testing/lisp/test-ob-emacs-lisp.el
+++ b/testing/lisp/test-ob-emacs-lisp.el
@@ -76,6 +76,92 @@
 	      (buffer-substring-no-properties (line-beginning-position 2)
 					      (line-end-position 2))))))
 
+(defun test-ob-emacs-lisp-dynamic-lexical-text (expr lexical)
+  (concat "\n"
+	  "#+begin_src emacs-lisp :lexical " lexical " :results verbatim\n"
+	  (format "%S" expr) "\n"
+	  "#+end_src"))
+
+(defvar test-ob-emacs-lisp-dynamic-lexical-expr
+  '(let ((x 'dynamic))
+     (funcall
+      (let ((x 'lexical))
+	(lambda () x)))))
+
+(ert-deftest ob-emacs-lisp/dynamic-lexical-execute ()
+  (cl-flet ((execute (text)
+              (org-test-with-temp-text-in-file text
+		(org-babel-next-src-block)
+		(org-babel-execute-maybe)
+		(re-search-forward "results" nil t)
+		(re-search-forward ": " nil t)
+		(buffer-substring-no-properties (point) (point-at-eol)))))
+    ;;
+    (cl-flet ((text (lexical)
+		(test-ob-emacs-lisp-dynamic-lexical-text
+		 test-ob-emacs-lisp-dynamic-lexical-expr
+		 lexical)))
+      (should (string= "dynamic" (execute (text "no" ))))
+      (should (string= "lexical" (execute (text "yes")))))
+    ;;
+    (cl-flet ((text (lexical)
+		(test-ob-emacs-lisp-dynamic-lexical-text
+		 'x
+		 lexical)))
+      (should (string= "dynamic" (let ((x 'dynamic))
+				   (execute (text "no")))))
+      (should (string= "lexical" (execute (text "'((x . lexical))")))))
+    ;;
+    ;; Src block execution uses `eval'. `eval' does not dynamically
+    ;; bind `lexical-binding' to the value of its LEXICAL
+    ;; parameter. Hence, (eval 'lexical-binding LEXICAL) evaluates to
+    ;; the same value that just `lexical-binding' evaluates to, no
+    ;; matter what is given as the LEXICAL parameter to eval. So the
+    ;; following does not work as intended:
+    ;;
+    ;;(cl-flet ((text (lexical)
+    ;;		(test-ob-emacs-lisp-dynamic-lexical-text
+    ;;		 'lexical-binding
+    ;;		 lexical)))
+    ;;  (should (string= "nil"       (execute (text "no"        ))))
+    ;;  (should (string= "t"         (execute (text "yes"       ))))
+    ;;  (should (string= "((x . 0))" (execute (text "'((x . 0))")))))
+    ))
+
+(ert-deftest ob-emacs-lisp/dynamic-lexical-edit ()
+  (cl-flet ((execute (text)
+              (org-test-with-temp-text-in-file text
+		(org-babel-next-src-block)
+		(org-edit-src-code)
+		(goto-char (point-max))
+		(prog1 (eval-last-sexp 0)
+		  (org-edit-src-exit)))))
+    ;;
+    (cl-flet ((text (lexical)
+		(test-ob-emacs-lisp-dynamic-lexical-text
+		 test-ob-emacs-lisp-dynamic-lexical-expr
+		 lexical)))
+      (should (eq 'dynamic (execute (text "no" ))))
+      (should (eq 'lexical (execute (text "yes")))))
+    ;;
+    (cl-flet ((text (lexical)
+		(test-ob-emacs-lisp-dynamic-lexical-text
+		 'x
+		 lexical)))
+      (should (eq 'dynamic (let ((x 'dynamic))
+			     (execute (text "no")))))
+      (should (eq 'lexical (execute (text "'((x . lexical))")))))
+    ;;
+    (cl-flet ((text (lexical)
+    		(test-ob-emacs-lisp-dynamic-lexical-text
+    		 'lexical-binding
+    		 lexical)))
+      (should (equal nil        (execute (text "no"        ))))
+      (should (equal t          (execute (text "yes"       ))))
+      (should (equal '((x . 0)) (execute (text "'((x . 0))")))))
+    ))
+
+
 (provide 'test-ob-emacs-lisp)
 
  ;;; test-ob-emacs-lisp.el ends here
-- 
2.21.0

Re: [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument

[Nicolas Goaziou]

Hello,

Sebastian Miele <sebastian.miele@gmail.com> writes:

> * testing/lisp/test-ob-emacs-lisp.el
>   (test-ob-emacs-lisp-dynamic-lexical-text,
>   test-ob-emacs-lisp-dynamic-lexical-expr,
>   ob-emacs-lisp/dynamic-lexical-execute,
>   ob-emacs-lisp/dynamic-lexical-edit): Add tests that check the
>   correct handling of the :lexical header argument when executing
>   source blocks and when creating editing buffers for source blocks.

Thank you.

<nitpick>
However, your tests are very convoluted. It is better than no test, but
if, unfortunately, one of them fail in some distant future, it may take
more time understanding what happens in the test than actually fixing
the bug.

Would you mind rewriting them with simple macros like, e.g.,
`org-test-with-temp-text', and use as little helper functions as
possible? IMO, code repetition in tests is not a problem.
</nitpick>

Regards,

-- 
Nicolas Goaziou

Re: [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument

[Sebastian Miele]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> [...]
>
> <nitpick>
> However, your tests are very convoluted. It is better than no test, but
> if, unfortunately, one of them fail in some distant future, it may take
> more time understanding what happens in the test than actually fixing
> the bug.
>
> Would you mind rewriting them with simple macros like, e.g.,
> `org-test-with-temp-text', and use as little helper functions as
> possible? IMO, code repetition in tests is not a problem.
> </nitpick>

After some initial tooth-grinding, I did rewrite them, and actually like
the result more than the previous version. Thank you for the suggestion!

Here is the updated patch:

* testing/lisp/test-ob-emacs-lisp.el
  (ob-emacs-lisp/dynamic-lexical-execute,
  ob-emacs-lisp/dynamic-lexical-edit): Add tests that check the
  correct handling of the :lexical header argument when executing
  source blocks and when creating editing buffers for source blocks.
---
 testing/lisp/test-ob-emacs-lisp.el | 89 ++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/testing/lisp/test-ob-emacs-lisp.el b/testing/lisp/test-ob-emacs-lisp.el
index 078cad988..24a373f86 100644
--- a/testing/lisp/test-ob-emacs-lisp.el
+++ b/testing/lisp/test-ob-emacs-lisp.el
@@ -76,6 +76,95 @@
 	      (buffer-substring-no-properties (line-beginning-position 2)
 					      (line-end-position 2))))))
 
+(ert-deftest ob-emacs-lisp/dynamic-lexical-execute ()
+  (cl-flet ((execute (text)
+              (org-test-with-temp-text-in-file text
+		(org-babel-next-src-block)
+		(org-babel-execute-maybe)
+		(re-search-forward "results" nil t)
+		(re-search-forward ": " nil t)
+		(buffer-substring-no-properties (point) (point-at-eol)))))
+
+    (should (string= "dynamic" (execute "
+#+begin_src emacs-lisp :lexical no :results verbatim
+(let ((x 'dynamic)) (funcall (let ((x 'lexical)) (lambda () x))))
+#+end_src")))
+
+    (should (string= "lexical" (execute "
+#+begin_src emacs-lisp :lexical yes :results verbatim
+(let ((x 'dynamic)) (funcall (let ((x 'lexical)) (lambda () x))))
+#+end_src")))
+
+    (should (string= "dynamic" (let ((x 'dynamic)) (execute "
+#+begin_src emacs-lisp :lexical no :results verbatim
+x
+#+end_src"))))
+
+    (should (string= "lexical" (let ((x 'dynamic)) (execute "
+#+begin_src emacs-lisp :lexical '((x . lexical)) :results verbatim
+x
+#+end_src"))))
+
+    ;; Src block execution uses `eval'. As of 2019-02-26, `eval' does
+    ;; not dynamically bind `lexical-binding' to the value of its
+    ;; LEXICAL parameter. Hence, (eval 'lexical-binding LEXICAL)
+    ;; evaluates to the same value that just `lexical-binding'
+    ;; evaluates to, even if LEXICAL is different. So tests like the
+    ;; following do not work here:
+    ;;
+    ;; (should (string= "t" (execute "
+    ;; #+begin_src emacs-lisp :lexical yes :results verbatim
+    ;; lexical-binding
+    ;; #+end_src")))
+    ;;
+    ;; However, the corresponding test in
+    ;; `ob-emacs-lisp/dynamic-lexical-edit' does work.
+    ))
+
+(ert-deftest ob-emacs-lisp/dynamic-lexical-edit ()
+  (cl-flet ((execute (text)
+              (org-test-with-temp-text-in-file text
+		(org-babel-next-src-block)
+		(org-edit-src-code)
+		(goto-char (point-max))
+		(prog1 (eval-last-sexp 0)
+		  (org-edit-src-exit)))))
+
+    (should (eq 'dynamic (execute "
+#+begin_src emacs-lisp :lexical no :results verbatim
+(let ((x 'dynamic)) (funcall (let ((x 'lexical)) (lambda () x))))
+#+end_src")))
+
+    (should (eq 'lexical (execute "
+#+begin_src emacs-lisp :lexical yes :results verbatim
+(let ((x 'dynamic)) (funcall (let ((x 'lexical)) (lambda () x))))
+#+end_src")))
+
+    (should (eq 'dynamic (let ((x 'dynamic)) (execute "
+#+begin_src emacs-lisp :lexical no :results verbatim
+x
+#+end_src"))))
+
+    (should (eq 'lexical (let ((x 'dynamic)) (execute "
+#+begin_src emacs-lisp :lexical '((x . lexical)) :results verbatim
+x
+#+end_src"))))
+
+    (should (equal nil (execute "
+#+begin_src emacs-lisp :lexical no :results verbatim
+lexical-binding
+#+end_src")))
+
+    (should (equal t (execute "
+#+begin_src emacs-lisp :lexical yes :results verbatim
+lexical-binding
+#+end_src")))
+
+    (should (equal '((x . 0)) (execute "
+#+begin_src emacs-lisp :lexical '((x . 0)) :results verbatim
+lexical-binding
+#+end_src")))))
+
 (provide 'test-ob-emacs-lisp)
 
  ;;; test-ob-emacs-lisp.el ends here
-- 
2.21.0

Re: [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument

[Nicolas Goaziou]

Sebastian Miele <sebastian.miele@gmail.com> writes:

> After some initial tooth-grinding,

My intent is not to be negative. I do understand that writing tests
takes time.

However, from experience, having to fix a regression when your only
indication comes from a test you do not fully understand is a pain. To
see what I mean, have a look at tests in
"test-ob-header-arg-defaults.el" and imagine one of them fails…

> I did rewrite them, and actually like
> the result more than the previous version. Thank you for the suggestion!
>
> Here is the updated patch:

Thank you. I applied it.

Re: [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument

[Sebastian Miele]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> However, from experience, having to fix a regression when your only
> indication comes from a test you do not fully understand is a pain. To
> see what I mean, have a look at tests in
> "test-ob-header-arg-defaults.el" and imagine one of them fails…

After looking into test-ob-header-arg-defaults.el and its accompanying
Org file for two hours I still do not understand everything that's going
on there. You are right. And I now have a solid desire and strategy to
circumnavigate the production of such problems in the future. Thank you
for the input.

Best wishes!