Re: [O] [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument
Nicolas Goaziou 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!
Re: [O] [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument
Sebastian Miele 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: [O] [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument
Nicolas Goaziou writes: > [...] > > > 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. > 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: [O] [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument
Hello, Sebastian Miele 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. 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. Regards, -- Nicolas Goaziou
[O] [PATCH 2/2] test-ob-emacs-lisp: Test :lexical src block header argument
* 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
