* Bash results broken? @ 2022-12-16 16:22 Rudolf Adamkovič 2022-12-16 17:41 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Rudolf Adamkovič @ 2022-12-16 16:22 UTC (permalink / raw) To: emacs-orgmode Reproduction steps: #+BEGIN_SRC bash :results list echo 1 echo 2 echo 3 #+END_SRC Actual: #+RESULTS: - (1) - (2) - (3) Expected: #+RESULTS: - 1 - 2 - 3 Rudy -- "Logic is a science of the necessary laws of thought, without which no employment of the understanding and the reason takes place." -- Immanuel Kant, 1785 Rudolf Adamkovič <salutis@me.com> [he/him] Studenohorská 25 84103 Bratislava Slovakia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Bash results broken? 2022-12-16 16:22 Bash results broken? Rudolf Adamkovič @ 2022-12-16 17:41 ` Ihor Radchenko 2022-12-18 11:19 ` Ihor Radchenko 2022-12-21 6:17 ` ob-shell intentions and paperwork (was Bash results broken?) Matt 0 siblings, 2 replies; 41+ messages in thread From: Ihor Radchenko @ 2022-12-16 17:41 UTC (permalink / raw) To: Rudolf Adamkovič; +Cc: emacs-orgmode Rudolf Adamkovič <salutis@me.com> writes: > Reproduction steps: > > #+BEGIN_SRC bash :results list > echo 1 > echo 2 > echo 3 > #+END_SRC > ... Confirmed. It appears to be present at least since Org shipped with Emacs 27. Wondering how nobody noticed... We really need more tests. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Bash results broken? 2022-12-16 17:41 ` Ihor Radchenko @ 2022-12-18 11:19 ` Ihor Radchenko 2022-12-20 0:44 ` Rudolf Adamkovič 2022-12-21 6:17 ` ob-shell intentions and paperwork (was Bash results broken?) Matt 1 sibling, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2022-12-18 11:19 UTC (permalink / raw) To: Rudolf Adamkovič; +Cc: emacs-orgmode Ihor Radchenko <yantar92@posteo.net> writes: > Confirmed. Fixed on bugfix. https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=224799875 -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Bash results broken? 2022-12-18 11:19 ` Ihor Radchenko @ 2022-12-20 0:44 ` Rudolf Adamkovič 2022-12-20 3:40 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Rudolf Adamkovič @ 2022-12-20 0:44 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode Ihor Radchenko <yantar92@posteo.net> writes: > Fixed on bugfix. I can confirm that the fix works. Thank you! Rudy -- "Programming reliably -- must be an activity of an undeniably mathematical nature […] You see, mathematics is about thinking, and doing mathematics is always trying to think as well as possible." -- Edsger W. Dijkstra, 1981 Rudolf Adamkovič <salutis@me.com> [he/him] Studenohorská 25 84103 Bratislava Slovakia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Bash results broken? 2022-12-20 0:44 ` Rudolf Adamkovič @ 2022-12-20 3:40 ` Matt 2022-12-25 11:23 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2022-12-20 3:40 UTC (permalink / raw) To: "Rudolf Adamkovič"; +Cc: ihor radchenko, emacs-orgmode ---- On Mon, 19 Dec 2022 19:44:12 -0500 Rudolf Adamkovič wrote --- > Ihor Radchenko yantar92@posteo.net> writes: > > > Fixed on bugfix. > > I can confirm that the fix works. Thank you! I can confirm it works, too, for the 1 2 3 example. However, it puts double quotes around text: #+BEGIN_SRC sh :results list echo hello echo world echo ! #+END_SRC #+RESULTS: - "hello" - "world" - "!" I was working on other ob-shell related stuff and my Emacs got into a mixed state where =ob-shell/results-list= was defined, but the fix wasn't (or wasn't loaded). I fixed the failing test before I saw this thread and Ihor's fix. The way I solved it was to wrap either =org-babel-import-elisp-from-file= or =org-babel-sh-evaluate= like, (mapcar #'car (org-babel-import-elisp-from-file tmp-file)) or (mapcar #'car (org-babel-sh-evaluate session full-body params stdin cmdline)) Doing this (without Ihor's fix in place) returns: #+BEGIN_SRC sh :results list echo "hello" echo "world" echo "!" #+END_SRC #+RESULTS: - hello - world - ! The underlying issue, so far as I understand it, is that =org-babel-import-elisp-from-file= returns a list-of-lists. For example, ((1) (2) (3)) or (("hello") ("world") ("!")). I thought it appropriate to flatten it out to (1 2 3) or ("hello" "world" "!"). Independently from how I addressed the bug, I feel it raises an important question: how should :results list handle text/strings? I'm not sure which approach is more "appropriate". If I echo a string without quotes, as I did in the first example, should it return a list that's got quotes? The hello world example is a little contrived–it's good practice to use double-quotes. I think a more realistic use case would be listing a directory. In that case, I'd probably want the list of items to be unquoted: Original: #+BEGIN_SRC sh :results list ls /home/ahab/.emacs.d/straight/repos/org/testing/lisp/ | head -n 3 #+END_SRC #+RESULTS: - ("test-ob-awk.el") - ("test-ob-C.el") - ("test-ob-clojure.el") Ihor solution: #+BEGIN_SRC sh :results list ls /home/ahab/.emacs.d/straight/repos/org/testing/lisp/ | head -n 3 #+END_SRC #+RESULTS: - "test-ob-awk.el" - "test-ob-C.el" - "test-ob-clojure.el" MAPCAR solution: #+BEGIN_SRC sh :results list ls /home/ahab/.emacs.d/straight/repos/org/testing/lisp/ | head -n 3 #+END_SRC #+RESULTS: - test-ob-awk.el - test-ob-C.el - test-ob-clojure.el For :results list, the manual says, "Interpret the results as an Org list. If the result is a single value, create a list of one element". I don't find that clarifies what would be best. Thoughts? ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Bash results broken? 2022-12-20 3:40 ` Matt @ 2022-12-25 11:23 ` Ihor Radchenko 2022-12-26 2:25 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2022-12-25 11:23 UTC (permalink / raw) To: Matt; +Cc: Rudolf Adamkovič, emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 554 bytes --] Matt <matt@excalamus.com> writes: > For :results list, the manual says, "Interpret the results as an Org list. If the result is a single value, create a list of one element". I don't find that clarifies what would be best. > > Thoughts? In my patch, I followed the previous code branch: (list (if (stringp e) e (format "%S" e))) As you see, "%S" have been used previously for non-string results. I cannot find explanation in git log. That said, I think that it will be more consistent to leave strings specifically as is. See the attached patch. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-org-babel-insert-result-Output-strings-as-is-for-lis.patch --] [-- Type: text/x-patch, Size: 1736 bytes --] From 22ee116511a40cc9cbee02e66799fdeb3e81ba78 Mon Sep 17 00:00:00 2001 Message-Id: <22ee116511a40cc9cbee02e66799fdeb3e81ba78.1671967390.git.yantar92@posteo.net> From: Ihor Radchenko <yantar92@posteo.net> Date: Sun, 25 Dec 2022 14:20:48 +0300 Subject: [PATCH] org-babel-insert-result: Output strings as is for lists of strings * lisp/ob-core.el (org-babel-insert-result): Do not use %S format for lists of strings in :results list output. This is more consistent with single string output. Reported-by: Matt <matt@excalamus.com> Link: https://orgmode.org/list/1852d9eb52f.c4c534f9581400.7140516675874523594@excalamus.com --- lisp/ob-core.el | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index c2a367375..300c9d92f 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -2461,13 +2461,18 @@ (defun org-babel-insert-result (result &optional result-params info hash lang ex (insert (org-trim (org-list-to-org + ;; We arbitrarily choose to format non-strings + ;; as %S. (cons 'unordered (mapcar (lambda (e) (cond ((stringp e) (list e)) ((listp e) - (mapcar (lambda (x) (format "%S" x)) e)) + (mapcar + (lambda (x) + (if (stringp x) x (format "%S" x))) + e)) (t (list (format "%S" e))))) (if (listp result) result (split-string result "\n" t)))) -- 2.38.1 [-- Attachment #3: Type: text/plain, Size: 225 bytes --] -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: Bash results broken? 2022-12-25 11:23 ` Ihor Radchenko @ 2022-12-26 2:25 ` Matt 2022-12-26 9:26 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2022-12-26 2:25 UTC (permalink / raw) To: Ihor Radchenko; +Cc: "rudolf adamkovič", emacs-orgmode ---- On Sun, 25 Dec 2022 06:23:53 -0500 Ihor Radchenko wrote --- > As you see, "%S" have been used previously for non-string results. > I cannot find explanation in git log. > > That said, I think that it will be more consistent to leave strings > specifically as is. See the attached patch. That makes sense and works for me. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Bash results broken? 2022-12-26 2:25 ` Matt @ 2022-12-26 9:26 ` Ihor Radchenko 0 siblings, 0 replies; 41+ messages in thread From: Ihor Radchenko @ 2022-12-26 9:26 UTC (permalink / raw) To: Matt; +Cc: rudolf adamkovič, emacs-orgmode Matt <matt@excalamus.com> writes: > ---- On Sun, 25 Dec 2022 06:23:53 -0500 Ihor Radchenko wrote --- > > As you see, "%S" have been used previously for non-string results. > > I cannot find explanation in git log. > > > > That said, I think that it will be more consistent to leave strings > > specifically as is. See the attached patch. > > That makes sense and works for me. Applied onto bugfix. https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b11abb409 -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* ob-shell intentions and paperwork (was Bash results broken?) 2022-12-16 17:41 ` Ihor Radchenko 2022-12-18 11:19 ` Ihor Radchenko @ 2022-12-21 6:17 ` Matt 2022-12-27 20:48 ` Matt 1 sibling, 1 reply; 41+ messages in thread From: Matt @ 2022-12-21 6:17 UTC (permalink / raw) To: Ihor Radchenko; +Cc: "rudolf adamkovič", emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 1376 bytes --] ---- On Fri, 16 Dec 2022 12:41:45 -0500 Ihor Radchenko wrote --- > We really need more tests. I'm working on giving ob-shell a little bit of love. I wrote the worg documentation for it earlier this year. I tried to include examples of all coded functionality, including previously undocumented features. I think the examples would make for good tests since they document the present and expected behavior. Currently, though, I'm refactoring the ob-shell tests to remove dependency on ob-shell-test.org and to stop the suite from littering. After that, I intend to incorporate the worg examples as tests. I'd like to adopt ob-shell, life permitting. Sadly, I'm not confident I'm able to commit to that right now. So, if I can get the FSF paperwork cleared, hopefully, I can make little contributions here and there. I've included patches of what I've been doing. They're not ready for actual submission yet, both in quality and legality. However, I'm including them for feedback, to make sure I'm not getting way off base. Overall, I'd *really* like to implement async for ob-shell similar to ob-python. However, I feel like it'd be more responsible and respectful to clean up the tests and code a little first before proposing such a big change (looking at you, org-babel-shell-initialize). Can someone please assist me with the FSF paperwork off list? [-- Attachment #2: 0001-Remove-mixed-tabs-and-spaces.patch --] [-- Type: application/octet-stream, Size: 4824 bytes --] From 2bc7b05bcd8a6e74fec18d070a92d8ebecde69e6 Mon Sep 17 00:00:00 2001 From: Matt <matt@excalamus.com> Date: Tue, 20 Dec 2022 13:51:26 -0500 Subject: [PATCH 1/7] Remove mixed tabs and spaces Converted tabs to spaces on the premise that one or the other is better than both. --- testing/lisp/test-ob-shell.el | 58 +++++++++++++++++------------------ 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index b0d9beff4..816e93ac5 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -32,9 +32,9 @@ "Expanded shell bodies should not start with a blank line unless the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "echo 2" '()))) + (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "\n\necho 2" '())))) + (org-babel-expand-body:generic "\n\necho 2" '())))) (ert-deftest test-ob-shell/dont-error-on-empty-results () "Was throwing an elisp error when shell blocks threw errors and @@ -49,8 +49,8 @@ ob-comint.el, which was not previously tested." (should (listp res))) ;; Test multi-line input. (let ((result (org-babel-execute:sh - "if true \n then \n echo yes \n fi" - '((:session . "yes"))))) + "if true \n then \n echo yes \n fi" + '((:session . "yes"))))) (should result) (should (string= "yes" result)))) @@ -71,47 +71,47 @@ ob-comint.el, which was not previously tested." "No associative arrays for generic" (should (equal "first one second two third three" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block))))) + (org-test-at-id + "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" + (org-babel-next-src-block) + (org-trim (org-babel-execute-src-block))))) (should (equal "bread 2 kg spaghetti 20 cm milk 50 dl" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block)))))) + (org-test-at-id + "82320a48-3409-49d7-85c9-5de1c6d3ff87" + (org-babel-next-src-block) + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-assoc-arrays () "Bash associative arrays" (should (equal "two" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block))))) + (org-test-at-id + "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" + (org-babel-next-src-block 2) + (org-trim (org-babel-execute-src-block))))) ;; Bash associative arrays as strings for the row. (should (equal "20 cm" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block)))))) + (org-test-at-id + "82320a48-3409-49d7-85c9-5de1c6d3ff87" + (org-babel-next-src-block 2) + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/simple-list () "Test list variables in shell." ;; With bash, a list is turned into an array. (should (equal "2" - (org-test-with-temp-text - "#+BEGIN_SRC bash :results output :var l='(1 2)\necho ${l[1]}\n#+END_SRC" - (org-trim (org-babel-execute-src-block))))) + (org-test-with-temp-text + "#+BEGIN_SRC bash :results output :var l='(1 2)\necho ${l[1]}\n#+END_SRC" + (org-trim (org-babel-execute-src-block))))) ;; On sh, it is a string containing all values. (should (equal "1 2" - (org-test-with-temp-text - "#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC" - (org-trim (org-babel-execute-src-block)))))) + (org-test-with-temp-text + "#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/remote-with-stdin-or-cmdline () "Test :stdin and :cmdline with a remote directory." @@ -166,9 +166,9 @@ ob-comint.el, which was not previously tested." "Test :results table." (should (equal '(("I \"want\" it all")) - (org-test-with-temp-text - "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC" - (org-babel-execute-src-block))))) + (org-test-with-temp-text + "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC" + (org-babel-execute-src-block))))) (ert-deftest ob-shell/results-list () "Test :results list." -- 2.38.1 [-- Attachment #3: 0002-Split-test-ob-shell-dont-error-on-empty-results.patch --] [-- Type: application/octet-stream, Size: 1709 bytes --] From d7a0e0df2f3d217d081a1b7302b1cab49aa614d0 Mon Sep 17 00:00:00 2001 From: Matt <matt@excalamus.com> Date: Tue, 20 Dec 2022 13:53:44 -0500 Subject: [PATCH 2/7] Split test-ob-shell/dont-error-on-empty-results Fixes ambiguity in test name, docstring, and function. The docstring said, "Was throwing an elisp error when shell blocks threw errors and returned empty results." The test checked two things: 1) whether `org-babel-execute:sh` could handle empty results and 2) whether it handled babel errors gracefully. These aren't necessarily the same thing. This change splits the test into the two conditions it was actually testing. It also cleans up buffers created during testing when successful. --- testing/lisp/test-ob-shell.el | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 816e93ac5..625dc64b9 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -37,9 +37,11 @@ unless the body of the tangled block does." (org-babel-expand-body:generic "\n\necho 2" '())))) (ert-deftest test-ob-shell/dont-error-on-empty-results () - "Was throwing an elisp error when shell blocks threw errors and -returned empty results." - (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil)))) + (should (null (org-babel-execute:sh nil nil)))) + +(ert-deftest test-ob-shell/dont-error-on-babel-error () + (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) + (kill-buffer "*Org-Babel Error Output*"))) (ert-deftest test-ob-shell/session () "This also tests `org-babel-comint-with-output' in -- 2.38.1 [-- Attachment #4: 0003-Refactor-test-ob-shell-session.patch --] [-- Type: application/octet-stream, Size: 3667 bytes --] From 1637ee73ed2ead8fac258996b9e05f18bfc33136 Mon Sep 17 00:00:00 2001 From: Matt <matt@excalamus.com> Date: Tue, 20 Dec 2022 15:22:38 -0500 Subject: [PATCH 3/7] Refactor test-ob-shell/session - Rename test precisely - Rename comint from "yes" to test name* - Clean up on success Several notes have been placed indicating future refactoring. Namely, `org-babel-shell-initialize` should be a macro since it generates functions. * The session name should be refactored into a variable, e.g. `session-name`. However, when this is done, `org-babel-execute:sh` produces a type error. The `params` symbol in `org-babel-execute:sh` does not evaluate as a variable for some reason and, because of how `org-babel-execute:sh` is defined, it's difficult to debug why. --- lisp/ob-shell.el | 2 ++ testing/lisp/test-ob-shell.el | 26 ++++++++++++++++---------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el index d38d2d335..8da9aa738 100644 --- a/lisp/ob-shell.el +++ b/lisp/ob-shell.el @@ -68,6 +68,8 @@ alist element.") (defvar org-babel-prompt-command) +;; TODO refactor into macro. Currently violates (elisp) Coding +;; Conventions and is hard to debug. (defun org-babel-shell-initialize () "Define execution functions associated to shell names. This function has to be called whenever `org-babel-shell-names' diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 625dc64b9..ecdc461d3 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -29,8 +29,8 @@ (signal 'missing-test-dependency "Support for Shell code blocks")) (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () - "Expanded shell bodies should not start with a blank line -unless the body of the tangled block does." + "Expanded shell bodies should not start with a blank line unless +the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" @@ -43,18 +43,24 @@ unless the body of the tangled block does." (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest test-ob-shell/session () - "This also tests `org-babel-comint-with-output' in -ob-comint.el, which was not previously tested." - (let ((res (org-babel-execute:sh "echo 1; echo 2" '((:session . "yes"))))) +;; TODO refactor session name into variable after refactoring +;; org-babel-execute:sh. See comment there. +(ert-deftest test-ob-shell/session-parameter-results-in-list () + ;; single line input + (let ((res (org-babel-execute:sh "echo 1; echo 2" '((:session . "test-ob-shell/session-parameter-results-in-list"))))) (should res) (should (listp res))) - ;; Test multi-line input. + + ;; multi-line input (let ((result (org-babel-execute:sh - "if true \n then \n echo yes \n fi" - '((:session . "yes"))))) + "if true \n then \n echo test-ob-shell/session-parameter-results-in-list \n fi" + '((:session . "test-ob-shell/session-parameter-results-in-list"))))) (should result) - (should (string= "yes" result)))) + (should (string= "test-ob-shell/session-parameter-results-in-list" result))) + + ;; clean up + (let ((kill-buffer-query-functions nil)) + (kill-buffer "test-ob-shell/session-parameter-results-in-list"))) ; A list of tests using the samples in ob-shell-test.org (ert-deftest ob-shell/generic-uses-no-arrays () -- 2.38.1 [-- Attachment #5: 0004-Refactor-ob-shell-generic-uses-no-arrays.patch --] [-- Type: application/octet-stream, Size: 1955 bytes --] From e8a7a82ad10073172eef2ef9e215860af56feb82 Mon Sep 17 00:00:00 2001 From: Matt <matt@excalamus.com> Date: Tue, 20 Dec 2022 18:10:52 -0500 Subject: [PATCH 4/7] Refactor ob-shell/generic-uses-no-arrays - Introduce `multiline-string` macro - Rename from "ob-shell" to "test-ob-shell" - Remove external file dependency --- testing/lisp/test-ob-shell.el | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index ecdc461d3..9c80aa1af 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -23,6 +23,18 @@ ;; Template test file for Org tests ;;; Code: +(defmacro multiline-string (&rest body) + "Join BODY with newlines. + +BODY is made of successive strings. + + (multiline-string \"first\" \"second\" \"third\") + ⇒ \"first\\nsecond\\nthird\" + +\(fn BODY)" + (declare (indent nil) (debug t)) + `(string-join (quote ,body) "\n")) + (org-test-for-executable "sh") (require 'ob-core) (unless (featurep 'ob-shell) @@ -62,11 +74,18 @@ the body of the tangled block does." (let ((kill-buffer-query-functions nil)) (kill-buffer "test-ob-shell/session-parameter-results-in-list"))) -; A list of tests using the samples in ob-shell-test.org -(ert-deftest ob-shell/generic-uses-no-arrays () - "No arrays for generic" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block) +(ert-deftest test-ob-shell/generic-uses-no-arrays () + (org-test-with-temp-text + (multiline-string + "#+NAME: sample_array" + "| one |" + "| two |" + "| three |" + "" + "#+begin_src sh :exports results :results output :var array=sample_array" + "echo ${array}" + "<point>" + "#+end_src") (should (equal "one two three" (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-arrays () -- 2.38.1 [-- Attachment #6: 0005-Refactor-ob-shell-bash-uses-arrays.patch --] [-- Type: application/octet-stream, Size: 1606 bytes --] From a1782ab8b38850858c4502db8e0a4999845c13dd Mon Sep 17 00:00:00 2001 From: Matt <matt@excalamus.com> Date: Tue, 20 Dec 2022 18:16:17 -0500 Subject: [PATCH 5/7] Refactor ob-shell/bash-uses-arrays - Rename "ob-shell" to "test-ob-shell" - Remove external file dependency --- testing/lisp/test-ob-shell.el | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 9c80aa1af..0328a0926 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -88,10 +88,22 @@ the body of the tangled block does." "#+end_src") (should (equal "one two three" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-arrays () - "Bash arrays" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block 2) +(ert-deftest test-ob-shell/bash-uses-arrays () + "Bash will see a simple indexed array. In this test, we check that the +returned value is indeed only the first item of the array, as opposed to +the generic serialiation that will return all elements of the array as +a single string." + (org-test-with-temp-text + (multiline-string + "#+NAME: sample_array" + "| one |" + "| two |" + "| three |" + "" + "#+begin_src bash :exports results :results output :var array=sample_array" + "echo ${array}" + "<point>" + "#+end_src") (should (equal "one" (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/generic-uses-no-assoc-arrays () -- 2.38.1 [-- Attachment #7: 0006-Refactor-ob-shell-generic-uses-no-assoc-arrays.patch --] [-- Type: application/octet-stream, Size: 2854 bytes --] From cd4143fe2399b1b45383bab267adad617612fd03 Mon Sep 17 00:00:00 2001 From: Matt <matt@excalamus.com> Date: Tue, 20 Dec 2022 19:22:40 -0500 Subject: [PATCH 6/7] Refactor ob-shell/generic-uses-no-assoc-arrays - Split tests based on simple versus complex mapping - Remove external file dependency - Rename "ob-shell" to "test-ob-shell" - Move headlines and commentary to doc string --- testing/lisp/test-ob-shell.el | 53 ++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 0328a0926..ed73a1cd9 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -106,20 +106,45 @@ a single string." "#+end_src") (should (equal "one" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/generic-uses-no-assoc-arrays () - "No associative arrays for generic" - (should - (equal "first one second two third three" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block))))) - (should - (equal "bread 2 kg spaghetti 20 cm milk 50 dl" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block)))))) +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-simple-map () + "Generic shell: no special handing for key-value mapping table + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + (multiline-string + "#+NAME: sample_mapping_table" + "| first | one |" + "| second | two |" + "| third | three |" + "" + "#+begin_src sh :exports results :results output :var table=sample_mapping_table" + "echo ${table}" + "<point>" + "#+end_src") + (should + (equal "first one second two third three" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-3-columns () + "Associative array tests (more than 2 columns) + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + (multiline-string + "#+NAME: sample_big_table" + "| bread | 2 | kg |" + "| spaghetti | 20 | cm |" + "| milk | 50 | dl |" + "" + "#+begin_src sh :exports results :results output :var table=sample_big_table" + "echo ${table}" + "<point>" + "#+end_src") + (should + (equal "bread 2 kg spaghetti 20 cm milk 50 dl" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-assoc-arrays () "Bash associative arrays" -- 2.38.1 [-- Attachment #8: 0007-Refactor-ob-shell-bash-uses-assoc-arrays.patch --] [-- Type: application/octet-stream, Size: 2760 bytes --] From 42975ddf6e8bd3ba39328989e9baf016ab510077 Mon Sep 17 00:00:00 2001 From: Matt <matt@excalamus.com> Date: Wed, 21 Dec 2022 00:45:44 -0500 Subject: [PATCH 7/7] Refactor ob-shell/bash-uses-assoc-arrays - Split cases into separate tests - Rename "ob-shell" to "test-ob-shell" - Remove external file dependency --- testing/lisp/test-ob-shell.el | 51 ++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index ed73a1cd9..1404647b9 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -146,21 +146,46 @@ as a single string." (equal "bread 2 kg spaghetti 20 cm milk 50 dl" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-assoc-arrays () - "Bash associative arrays" - (should - (equal "two" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block))))) - ;; Bash associative arrays as strings for the row. +(ert-deftest test-ob-shell/bash-uses-assoc-arrays () + "Bash shell: support for associative arrays + +Bash will see a table that contains the first column as the +'index' of the associative array, and the second column as the +value. " + (org-test-with-temp-text + (multiline-string + "#+NAME: sample_mapping_table" + "| first | one |" + "| second | two |" + "| third | three |" + "" + "#+begin_src bash :exports :results output results :var table=sample_mapping_table" + "echo ${table[second]}" + "<point>" + "#+end_src") + (should + (equal "two" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/bash-uses-assoc-arrays-with-lists () + "Bash shell: support for associative arrays with lists + +Bash will see an associative array that contains each row as a single +string. Bash cannot handle lists in associative arrays." + (org-test-with-temp-text + (multiline-string + "#+NAME: sample_big_table" + "| bread | 2 | kg |" + "| spaghetti | 20 | cm |" + "| milk | 50 | dl |" + "" + "#+begin_src bash :exports results :results output :var table=sample_big_table" + "echo ${table[spaghetti]}" + "<point>" + "#+end_src") (should (equal "20 cm" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block)))))) + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/simple-list () "Test list variables in shell." -- 2.38.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-21 6:17 ` ob-shell intentions and paperwork (was Bash results broken?) Matt @ 2022-12-27 20:48 ` Matt 2022-12-29 11:08 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2022-12-27 20:48 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 855 bytes --] ---- On Wed, 21 Dec 2022 01:17:50 -0500 Matt wrote --- > Currently, though, I'm refactoring the ob-shell tests to remove dependency on ob-shell-test.org and to stop the suite from littering. Done. Branched off bugfix, 12e10eb0d, and refactored test-ob-shell.el. See attached patches. The main changes spanning the patches are: - tests are now standalone; they don't rely on ob-shell-test.org - tests now share a common prefix, "test-ob-shell". The entire suite runs with (ert "test-ob-shell") - buffers and processes created during a test are killed when the test passes > After that, I intend to incorporate the worg examples as tests. This is probably a good place for me to pause and get feedback before writing more code. I just got the signed paperwork back from Craig and am waiting to be admitted to the Emacs group on Savannah. [-- Attachment #2: 0001-test-ob-shell.el-Remove-mixed-tabs-and-spaces.patch --] [-- Type: application/octet-stream, Size: 4889 bytes --] From ecdb71afa00ca137b4faa737393cb027907a7f9f Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Tue, 20 Dec 2022 13:51:26 -0500 Subject: [PATCH 01/20] test-ob-shell.el: Remove mixed tabs and spaces * test-ob-shell.el: Convert tabs to spaces. Change made under the premise that one or the other is better than both. --- testing/lisp/test-ob-shell.el | 58 +++++++++++++++++------------------ 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index b0d9beff4..816e93ac5 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -32,9 +32,9 @@ "Expanded shell bodies should not start with a blank line unless the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "echo 2" '()))) + (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "\n\necho 2" '())))) + (org-babel-expand-body:generic "\n\necho 2" '())))) (ert-deftest test-ob-shell/dont-error-on-empty-results () "Was throwing an elisp error when shell blocks threw errors and @@ -49,8 +49,8 @@ ob-comint.el, which was not previously tested." (should (listp res))) ;; Test multi-line input. (let ((result (org-babel-execute:sh - "if true \n then \n echo yes \n fi" - '((:session . "yes"))))) + "if true \n then \n echo yes \n fi" + '((:session . "yes"))))) (should result) (should (string= "yes" result)))) @@ -71,47 +71,47 @@ ob-comint.el, which was not previously tested." "No associative arrays for generic" (should (equal "first one second two third three" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block))))) + (org-test-at-id + "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" + (org-babel-next-src-block) + (org-trim (org-babel-execute-src-block))))) (should (equal "bread 2 kg spaghetti 20 cm milk 50 dl" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block)))))) + (org-test-at-id + "82320a48-3409-49d7-85c9-5de1c6d3ff87" + (org-babel-next-src-block) + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-assoc-arrays () "Bash associative arrays" (should (equal "two" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block))))) + (org-test-at-id + "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" + (org-babel-next-src-block 2) + (org-trim (org-babel-execute-src-block))))) ;; Bash associative arrays as strings for the row. (should (equal "20 cm" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block)))))) + (org-test-at-id + "82320a48-3409-49d7-85c9-5de1c6d3ff87" + (org-babel-next-src-block 2) + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/simple-list () "Test list variables in shell." ;; With bash, a list is turned into an array. (should (equal "2" - (org-test-with-temp-text - "#+BEGIN_SRC bash :results output :var l='(1 2)\necho ${l[1]}\n#+END_SRC" - (org-trim (org-babel-execute-src-block))))) + (org-test-with-temp-text + "#+BEGIN_SRC bash :results output :var l='(1 2)\necho ${l[1]}\n#+END_SRC" + (org-trim (org-babel-execute-src-block))))) ;; On sh, it is a string containing all values. (should (equal "1 2" - (org-test-with-temp-text - "#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC" - (org-trim (org-babel-execute-src-block)))))) + (org-test-with-temp-text + "#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/remote-with-stdin-or-cmdline () "Test :stdin and :cmdline with a remote directory." @@ -166,9 +166,9 @@ ob-comint.el, which was not previously tested." "Test :results table." (should (equal '(("I \"want\" it all")) - (org-test-with-temp-text - "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC" - (org-babel-execute-src-block))))) + (org-test-with-temp-text + "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC" + (org-babel-execute-src-block))))) (ert-deftest ob-shell/results-list () "Test :results list." -- 2.38.1 [-- Attachment #3: 0002-test-ob-shell.el-Split-cases-into-two-tests.patch --] [-- Type: application/octet-stream, Size: 1754 bytes --] From 3f712ecc2302a16514d0ce4b2dbb98f0330bc8a2 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Tue, 20 Dec 2022 13:53:44 -0500 Subject: [PATCH 02/20] test-ob-shell.el: Split cases into two tests * testing/lisp/test-ob-shell.el (test-ob-shell/dont-error-on-empty-results): Fix ambiguity in test name, docstring, and function. Explicitly test for empty results. (test-ob-shell/dont-error-on-babel-error): Explicitly test for babel error handling. Previously tested implicitly. On pass, remove buffers created during test. Two sources of errors exist: 1) empty results and 2) babel errors. Original test checked both, despite its name specifing only the first type. Change splits original test into two according type of error. --- testing/lisp/test-ob-shell.el | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 816e93ac5..625dc64b9 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -37,9 +37,11 @@ unless the body of the tangled block does." (org-babel-expand-body:generic "\n\necho 2" '())))) (ert-deftest test-ob-shell/dont-error-on-empty-results () - "Was throwing an elisp error when shell blocks threw errors and -returned empty results." - (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil)))) + (should (null (org-babel-execute:sh nil nil)))) + +(ert-deftest test-ob-shell/dont-error-on-babel-error () + (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) + (kill-buffer "*Org-Babel Error Output*"))) (ert-deftest test-ob-shell/session () "This also tests `org-babel-comint-with-output' in -- 2.38.1 [-- Attachment #4: 0003-test-ob-shell.el-Refactor-test-ob-shell-session.patch --] [-- Type: application/octet-stream, Size: 4652 bytes --] From c64354854f6016cb0ad07c90f5e474c97cd790db Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Tue, 20 Dec 2022 15:22:38 -0500 Subject: [PATCH 03/20] test-ob-shell.el: Refactor test-ob-shell/session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * testing/lisp/test-ob-shell.el (ob-shell/session): Split `ob-shell/session' into `test-ob-shell/session-single-return-returns-string' and `test-ob-shell/session-multiple-returns-returns-list'. (ob-shell/session): Rename comint from "yes" to test name[fn:1]. On pass, kill process and remove process buffer. * testing/lisp/test-ob-shell.el, lisp/ob-shell.el: Place notes indicating need for future refactoring. [fn:1] Session name is a string and not a variable (such as `session-name'). This is because `org-babel-execute:sh' produces a type error when the session name is defined with a variable. The source of the error appears to be the `params' symbol in `org-babel-execute:sh'. `params' does not evaluate as a variable as expected–it evaluates as a symbol. However, `org-babel-execute:sh' is defined within a function factory which makes it difficult to debug. Hard code the test name for now and refactor it later once `org-babel-execute:sh` is refactored. --- lisp/ob-shell.el | 2 ++ testing/lisp/test-ob-shell.el | 36 ++++++++++++++++++++++------------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el index d38d2d335..8da9aa738 100644 --- a/lisp/ob-shell.el +++ b/lisp/ob-shell.el @@ -68,6 +68,8 @@ alist element.") (defvar org-babel-prompt-command) +;; TODO refactor into macro. Currently violates (elisp) Coding +;; Conventions and is hard to debug. (defun org-babel-shell-initialize () "Define execution functions associated to shell names. This function has to be called whenever `org-babel-shell-names' diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 625dc64b9..54b6d5a6f 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -29,8 +29,8 @@ (signal 'missing-test-dependency "Support for Shell code blocks")) (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () - "Expanded shell bodies should not start with a blank line -unless the body of the tangled block does." + "Expanded shell bodies should not start with a blank line unless +the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" @@ -43,18 +43,28 @@ unless the body of the tangled block does." (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest test-ob-shell/session () - "This also tests `org-babel-comint-with-output' in -ob-comint.el, which was not previously tested." - (let ((res (org-babel-execute:sh "echo 1; echo 2" '((:session . "yes"))))) - (should res) - (should (listp res))) - ;; Test multi-line input. - (let ((result (org-babel-execute:sh - "if true \n then \n echo yes \n fi" - '((:session . "yes"))))) +;; TODO refactor session name into variable after refactoring +;; org-babel-execute:sh. See comment there. +(ert-deftest test-ob-shell/session-single-return-returns-string () + (let ((kill-buffer-query-functions nil) + (result (org-babel-execute:sh + "echo test-ob-shell/session-evaluation-single-return-returns-string" + '((:session . "test-ob-shell/session-evaluation-single-return-returns-string"))))) (should result) - (should (string= "yes" result)))) + (if (should (string= "test-ob-shell/session-evaluation-single-return-returns-string" result)) + (kill-buffer "test-ob-shell/session-evaluation-single-return-returns-string")))) + +;; TODO refactor session name into variable after refactoring +;; org-babel-execute:sh. See comment there. +(ert-deftest test-ob-shell/session-multiple-returns-returns-list () + (let ((kill-buffer-query-functions nil) + (result (org-babel-execute:sh + "echo 1; echo 2" + '((:session . "test-ob-shell/session-multiple-returns-returns-list"))))) + (should result) + (should (listp result)) + (if (should (equal '((1) (2)) result)) + (kill-buffer "test-ob-shell/session-multiple-returns-returns-list")))) ; A list of tests using the samples in ob-shell-test.org (ert-deftest ob-shell/generic-uses-no-arrays () -- 2.38.1 [-- Attachment #5: 0004-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch --] [-- Type: application/octet-stream, Size: 2906 bytes --] From 55caaf5c0fe4f2e9fe4b1c3cb1740ec2a6072c7b Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Tue, 20 Dec 2022 18:10:52 -0500 Subject: [PATCH 04/20] test-ob-shell.el: Refactor ob-shell/generic-uses-no-arrays * testing/lisp/test-ob-shell.el: Define function `test-ob-shell-multiline-string'. (ob-shell/generic-uses-no-arrays): Rename test from `ob-shell/generic-uses-no-arrays' to `test-ob-shell/generic-uses-no-arrays'. (ob-shell/generic-uses-no-arrays): Explain test in doc string. (ob-shell/generic-uses-no-arrays): Remove dependency on testing/examples/ob-shell-test.org. --- testing/lisp/test-ob-shell.el | 41 +++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 54b6d5a6f..df32b8aa7 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -20,17 +20,32 @@ ;;; Comment: -;; Template test file for Org tests - -;;; Code: +;;; Requirements: (org-test-for-executable "sh") + (require 'ob-core) + (unless (featurep 'ob-shell) (signal 'missing-test-dependency "Support for Shell code blocks")) +;;; Code: +(defun test-ob-shell-multiline-string (&rest strings) + "Join STRINGS with newlines. + +Each expression of STRINGS should evaluate to a string. + + (test-ob-shell-multiline-string + \"first\" + (format \"second\") + (let ((last \"third\")) last)) + ⇒ \"first\\nsecond\\nthird\" + +\(fn STRINGS)" + (string-join strings "\n")) + (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () "Expanded shell bodies should not start with a blank line unless -the body of the tangled block does." +the strings of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" @@ -66,11 +81,19 @@ the body of the tangled block does." (if (should (equal '((1) (2)) result)) (kill-buffer "test-ob-shell/session-multiple-returns-returns-list")))) -; A list of tests using the samples in ob-shell-test.org -(ert-deftest ob-shell/generic-uses-no-arrays () - "No arrays for generic" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block) +(ert-deftest test-ob-shell/generic-uses-no-arrays () + "Test generic serialization of array into a single string." + (org-test-with-temp-text + (test-ob-shell-multiline-string + "#+NAME: sample_array" + "| one |" + "| two |" + "| three |" + "" + "#+begin_src sh :exports results :results output :var array=sample_array" + "echo ${array}" + "<point>" + "#+end_src") (should (equal "one two three" (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-arrays () -- 2.38.1 [-- Attachment #6: 0005-test-ob-shell.el-Refactor-ob-shell-bash-uses-arrays.patch --] [-- Type: application/octet-stream, Size: 1856 bytes --] From ebc48d2e7c3314d9669d212a237e319787974ab8 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Tue, 20 Dec 2022 18:16:17 -0500 Subject: [PATCH 05/20] test-ob-shell.el: Refactor ob-shell/bash-uses-arrays * test-ob-shell.el (ob-shell/bash-uses-arrays): Rename `ob-shell/bash-uses-arrays' to `test-ob-shell/bash-uses-arrays'. (ob-shell/bash-uses-arrays): Explain test in docstring. (ob-shell/bash-uses-arrays): Remove dependency on testing/examples/ob-shell-test.org. --- testing/lisp/test-ob-shell.el | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index df32b8aa7..4c70c07fb 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -96,10 +96,23 @@ the strings of the tangled block does." "#+end_src") (should (equal "one two three" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-arrays () - "Bash arrays" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block 2) +(ert-deftest test-ob-shell/bash-uses-arrays () + "Bash sees named array as a simple indexed array. + +In this test, we check that the returned value is indeed only the +first item of the array, as opposed to the generic serialiation +that will return all elements of the array as a single string." + (org-test-with-temp-text + (test-ob-shell-multiline-string + "#+NAME: sample_array" + "| one |" + "| two |" + "| three |" + "" + "#+begin_src bash :exports results :results output :var array=sample_array" + "echo ${array}" + "<point>" + "#+end_src") (should (equal "one" (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/generic-uses-no-assoc-arrays () -- 2.38.1 [-- Attachment #7: 0006-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch --] [-- Type: application/octet-stream, Size: 3296 bytes --] From 021a950bff733730be99a3a9a124c5e66de66b4e Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Tue, 20 Dec 2022 19:22:40 -0500 Subject: [PATCH 06/20] test-ob-shell.el: Refactor ob-shell/generic-uses-no-assoc-arrays * test-ob-shell.el (ob-shell/generic-uses-no-assoc-arrays): Move headlines and commentary from `testing/examples/ob-shell-test.org' to docstring. (ob-shell/generic-uses-no-assoc-arrays): Remove dependency on testing/examples/ob-shell-test.org. (ob-shell/generic-uses-no-assoc-arrays): Split test based on simple versus complex mapping cases, `test-ob-shell/generic-uses-no-assoc-arrays-simple-map' and `test-ob-shell/generic-uses-no-assoc-arrays-3-columns'. Rename prefix from "ob-shell" to "test-ob-shell". --- testing/lisp/test-ob-shell.el | 53 ++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 4c70c07fb..5c77b088b 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -115,20 +115,45 @@ that will return all elements of the array as a single string." "#+end_src") (should (equal "one" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/generic-uses-no-assoc-arrays () - "No associative arrays for generic" - (should - (equal "first one second two third three" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block))))) - (should - (equal "bread 2 kg spaghetti 20 cm milk 50 dl" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block)))))) +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-simple-map () + "Generic shell: no special handing for key-value mapping table + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + (test-ob-shell-multiline-string + "#+NAME: sample_mapping_table" + "| first | one |" + "| second | two |" + "| third | three |" + "" + "#+begin_src sh :exports results :results output :var table=sample_mapping_table" + "echo ${table}" + "<point>" + "#+end_src") + (should + (equal "first one second two third three" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-3-columns () + "Associative array tests (more than 2 columns) + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + (test-ob-shell-multiline-string + "#+NAME: sample_big_table" + "| bread | 2 | kg |" + "| spaghetti | 20 | cm |" + "| milk | 50 | dl |" + "" + "#+begin_src sh :exports results :results output :var table=sample_big_table" + "echo ${table}" + "<point>" + "#+end_src") + (should + (equal "bread 2 kg spaghetti 20 cm milk 50 dl" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-assoc-arrays () "Bash associative arrays" -- 2.38.1 [-- Attachment #8: 0007-test-ob-shell.el-Refactor-ob-shell-bash-uses-assoc-a.patch --] [-- Type: application/octet-stream, Size: 3158 bytes --] From 5b24aa18ec1fe8d60968d6530b2373d487bd8e9f Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Wed, 21 Dec 2022 00:45:44 -0500 Subject: [PATCH 07/20] test-ob-shell.el: Refactor ob-shell/bash-uses-assoc-arrays * testing/lisp/test-ob-shell.el (ob-shell/bash-uses-assoc-arrays): Split test cases into separate tests, `test-ob-shell/bash-uses-assoc-arrays' and `test-ob-shell/bash-uses-assoc-arrays-with-lists'. Rename prefix from "ob-shell" to "test-ob-shell". (ob-shell/bash-uses-assoc-arrays): Move comments from testing/examples/ob-shell-test.org to docstrings. (ob-shell/bash-uses-assoc-arrays): Remove dependency on testing/examples/ob-shell-test.org. --- testing/lisp/test-ob-shell.el | 51 ++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 5c77b088b..a1cacdc16 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -155,21 +155,46 @@ as a single string." (equal "bread 2 kg spaghetti 20 cm milk 50 dl" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-assoc-arrays () - "Bash associative arrays" - (should - (equal "two" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block))))) - ;; Bash associative arrays as strings for the row. +(ert-deftest test-ob-shell/bash-uses-assoc-arrays () + "Bash shell: support for associative arrays + +Bash will see a table that contains the first column as the +'index' of the associative array, and the second column as the +value. " + (org-test-with-temp-text + (test-ob-shell-multiline-string + "#+NAME: sample_mapping_table" + "| first | one |" + "| second | two |" + "| third | three |" + "" + "#+begin_src bash :exports :results output results :var table=sample_mapping_table" + "echo ${table[second]}" + "<point>" + "#+end_src") + (should + (equal "two" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/bash-uses-assoc-arrays-with-lists () + "Bash shell: support for associative arrays with lists + +Bash will see an associative array that contains each row as a single +string. Bash cannot handle lists in associative arrays." + (org-test-with-temp-text + (test-ob-shell-multiline-string + "#+NAME: sample_big_table" + "| bread | 2 | kg |" + "| spaghetti | 20 | cm |" + "| milk | 50 | dl |" + "" + "#+begin_src bash :exports results :results output :var table=sample_big_table" + "echo ${table[spaghetti]}" + "<point>" + "#+end_src") (should (equal "20 cm" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block)))))) + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/simple-list () "Test list variables in shell." -- 2.38.1 [-- Attachment #9: 0008-test-ob-shell.el-Refactor-ob-shell-simple-list.patch --] [-- Type: application/octet-stream, Size: 2151 bytes --] From c05e7eaace70589546083c84ae78369c613c532d Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 21:16:42 -0500 Subject: [PATCH 08/20] test-ob-shell.el: Refactor ob-shell/simple-list * test-ob-shell.el (ob-shell/simple-list): Change test name from `ob-shell/simple-list' to `test-ob-shell/simple-list'. (ob-shell/simple-list): Use `test-ob-shell-multiline-string' to write source block on multiple lines. --- testing/lisp/test-ob-shell.el | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index a1cacdc16..085dc694d 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -196,20 +196,27 @@ string. Bash cannot handle lists in associative arrays." (equal "20 cm" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/simple-list () - "Test list variables in shell." - ;; With bash, a list is turned into an array. +(ert-deftest test-ob-shell/simple-list () + "Test list variables." + ;; bash: a list is turned into an array (should (equal "2" (org-test-with-temp-text - "#+BEGIN_SRC bash :results output :var l='(1 2)\necho ${l[1]}\n#+END_SRC" + (test-ob-shell-multiline-string + "#+BEGIN_SRC bash :results output :var l='(1 2)" + "echo ${l[1]}" + "#+END_SRC") (org-trim (org-babel-execute-src-block))))) - ;; On sh, it is a string containing all values. + + ;; sh: a list is a string containing all values (should (equal "1 2" (org-test-with-temp-text - "#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC" - (org-trim (org-babel-execute-src-block)))))) + (test-ob-shell-multiline-string + "#+BEGIN_SRC sh :results output :var l='(1 2)" + "echo ${l}" + "#+END_SRC") + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/remote-with-stdin-or-cmdline () "Test :stdin and :cmdline with a remote directory." -- 2.38.1 [-- Attachment #10: 0009-test-ob-shell.el-Refactor-ob-shell-remote-with-stdin.patch --] [-- Type: application/octet-stream, Size: 1819 bytes --] From 2178b926cd9b2a3ee01e6fc6d0282372f4ff72bd Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 21:26:16 -0500 Subject: [PATCH 09/20] test-ob-shell.el: Refactor ob-shell/remote-with-stdin-or-cmdline * test-ob-shell.el (ob-shell/remote-with-stdin-or-cmdline): Change test name from `ob-shell/remote-with-stdin-or-cmdline' to `test-ob-shell/remote-with-stdin-or-cmdline'. (ob-shell/remote-with-stdin-or-cmdline): When test passes, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 085dc694d..d32cc9002 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -218,7 +218,7 @@ string. Bash cannot handle lists in associative arrays." "#+END_SRC") (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/remote-with-stdin-or-cmdline () +(ert-deftest test-ob-shell/remote-with-stdin-or-cmdline () "Test :stdin and :cmdline with a remote directory." ;; We assume `default-directory' is a local directory. (skip-unless (not (memq system-type '(ms-dos windows-nt)))) @@ -265,7 +265,8 @@ string. Bash cannot handle lists in associative arrays." (org-trim (org-babel-execute-src-block)))) (expected (concat "ARGS: --verbose 23 71" "\nhello tramp from " (file-local-name default-directory)))) - (should (equal result expected))))))) + (if (should (equal result expected)) + (kill-matching-buffers (format "\\*tramp/mock\\s-%s\\*" system-name) t t))))))) (ert-deftest ob-shell/results-table () "Test :results table." -- 2.38.1 [-- Attachment #11: 0010-test-ob-shell.el-Refactor-ob-shell-results-table.patch --] [-- Type: application/octet-stream, Size: 1488 bytes --] From 02f510153a252655da8fe11ab662f59bdf281a21 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:14:19 -0500 Subject: [PATCH 10/20] test-ob-shell.el: Refactor ob-shell/results-table * test-ob-shell.el (ob-shell/results-table): Rename `ob-shell/results-table' to `test-ob-shell/results-table'. (ob-shell/results-table): Use `test-ob-shell-multiline-string' to write source block on multiple lines. --- testing/lisp/test-ob-shell.el | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index d32cc9002..87a10eaa3 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -268,12 +268,15 @@ string. Bash cannot handle lists in associative arrays." (if (should (equal result expected)) (kill-matching-buffers (format "\\*tramp/mock\\s-%s\\*" system-name) t t))))))) -(ert-deftest ob-shell/results-table () +(ert-deftest test-ob-shell/results-table () "Test :results table." (should (equal '(("I \"want\" it all")) (org-test-with-temp-text - "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC" + (test-ob-shell-multiline-string + "#+BEGIN_SRC sh :results table" + "echo 'I \"want\" it all'" + "#+END_SRC") (org-babel-execute-src-block))))) (ert-deftest ob-shell/results-list () -- 2.38.1 [-- Attachment #12: 0011-test-ob-shell.el-Refactor-ob-shell-results-list.patch --] [-- Type: application/octet-stream, Size: 1323 bytes --] From eef3d9ade6171c5469a366e4dffee0c63c949458 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:34:12 -0500 Subject: [PATCH 11/20] test-ob-shell.el: Refactor ob-shell/results-list * test-ob-shell.el (ob-shell/results-list): Rename `ob-shell/results-list' to `test-ob-shell/results-list'. (ob-shell/results-list): Use `test-ob-shell-multiline-string' to write source block on multiple lines. --- testing/lisp/test-ob-shell.el | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 87a10eaa3..93f4c45af 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -279,10 +279,15 @@ string. Bash cannot handle lists in associative arrays." "#+END_SRC") (org-babel-execute-src-block))))) -(ert-deftest ob-shell/results-list () +(ert-deftest test-ob-shell/results-list () "Test :results list." (org-test-with-temp-text - "#+BEGIN_SRC sh :results list\necho 1\necho 2\necho 3\n#+END_SRC" + (test-ob-shell-multiline-string + "#+BEGIN_SRC sh :results list" + "echo 1" + "echo 2" + "echo 3" + "#+END_SRC") (should (equal '((1) (2) (3)) (org-babel-execute-src-block))) -- 2.38.1 [-- Attachment #13: 0012-test-ob-shell.el-Refactor-ob-shell-standard-output-a.patch --] [-- Type: application/octet-stream, Size: 1013 bytes --] From 49378281e970d434d004c9e0ebc21f57dd0c3f56 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:36:51 -0500 Subject: [PATCH 12/20] test-ob-shell.el: Refactor ob-shell/standard-output-after-success * test-ob-shell.el (ob-shell/standard-output-after-success): Rename `ob-shell/standard-output-after-success' to `test-ob-shell/standard-output-after-success'. --- testing/lisp/test-ob-shell.el | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 93f4c45af..a3b2e9ee0 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -300,7 +300,7 @@ string. Bash cannot handle lists in associative arrays." ;;; Standard output -(ert-deftest ob-shell/standard-output-after-success () +(ert-deftest test-ob-shell/standard-output-after-success () "Test standard output after exiting with a zero code." (should (= 1 (org-babel-execute:sh -- 2.38.1 [-- Attachment #14: 0013-test-ob-shell.el-Refactor-ob-shell-standard-output-a.patch --] [-- Type: application/octet-stream, Size: 1383 bytes --] From ec9ae8226a895b1aecbb9037456b52649295563a Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:38:00 -0500 Subject: [PATCH 13/20] test-ob-shell.el: Refactor ob-shell/standard-output-after-failure * test-ob-shell.el (ob-shell/standard-output-after-failure): Rename `ob-shell/standard-output-after-failure' to `test-ob-shell/standard-output-after-failure'. (ob-shell/standard-output-after-failure): On pass, kill buffer created during test. --- testing/lisp/test-ob-shell.el | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index a3b2e9ee0..1742cf16f 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -306,11 +306,13 @@ string. Bash cannot handle lists in associative arrays." (org-babel-execute:sh "echo 1" nil)))) -(ert-deftest ob-shell/standard-output-after-failure () +(ert-deftest test-ob-shell/standard-output-after-failure () "Test standard output after exiting with a non-zero code." - (should (= 1 - (org-babel-execute:sh - "echo 1; exit 2" nil)))) + (if + (should (= 1 + (org-babel-execute:sh + "echo 1; exit 2" nil))) + (kill-buffer "*Org-Babel Error Output*"))) ;;; Standard error -- 2.38.1 [-- Attachment #15: 0014-test-ob-shell.el-Refactor-ob-shell-error-output-afte.patch --] [-- Type: application/octet-stream, Size: 1087 bytes --] From 0e643556f41e1a14b77d9973b2e1034fd5f22da8 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:39:11 -0500 Subject: [PATCH 14/20] test-ob-shell.el: Refactor ob-shell/error-output-after-success * test-ob-shell.el (ob-shell/error-output-after-success): Rename `ob-shell/error-output-after-success' to `test-ob-shell/error-output-after-success'. (ob-shell/error-output-after-success): On pass, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 1742cf16f..9d0dea537 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -316,7 +316,7 @@ string. Bash cannot handle lists in associative arrays." ;;; Standard error -(ert-deftest ob-shell/error-output-after-success () +(ert-deftest test-ob-shell/error-output-after-success () "Test that standard error shows in the error buffer, alongside the exit code, after exiting with a zero code." (should -- 2.38.1 [-- Attachment #16: 0015-test-ob-shell.el-Refactor-ob-shell-error-output-afte.patch --] [-- Type: application/octet-stream, Size: 2766 bytes --] From 0b72f870ec8f079ab5f00d02f7768797e57a40b8 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:53:33 -0500 Subject: [PATCH 15/20] test-ob-shell.el: Refactor ob-shell/error-output-after-failure * test-ob-shell.el (ob-shell/error-output-after-failure): Rename `ob-shell/error-output-after-failure' to `test-ob-shell/error-output-after-failure'. (ob-shell/error-output-after-failure): On pass, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 9d0dea537..9ba09908b 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -319,26 +319,30 @@ string. Bash cannot handle lists in associative arrays." (ert-deftest test-ob-shell/error-output-after-success () "Test that standard error shows in the error buffer, alongside the exit code, after exiting with a zero code." - (should - (string= "1 + (if + (should + (string= "1 [ Babel evaluation exited with code 0 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest ob-shell/error-output-after-failure () +(ert-deftest test-ob-shell/error-output-after-failure () "Test that standard error shows in the error buffer, alongside the exit code, after exiting with a non-zero code." - (should - (string= "1 + (if + (should + (string= "1 [ Babel evaluation exited with code 2 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2; exit 2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2; exit 2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) (ert-deftest ob-shell/error-output-after-failure-multiple () "Test that multiple standard error strings show in the error -- 2.38.1 [-- Attachment #17: 0016-test-ob-shell.el-Refactor-ob-shell-error-output-afte.patch --] [-- Type: application/octet-stream, Size: 2163 bytes --] From f2f56a6919d877e36d95ab33dce3b1fcd1e9bcf9 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:55:34 -0500 Subject: [PATCH 16/20] test-ob-shell.el: Refactor ob-shell/error-output-after-failure-multiple * test-ob-shell.el (ob-shell/error-output-after-failure-multiple): Rename `ob-shell/error-output-after-failure-multiple' to `test-ob-shell/error-output-after-failure-multiple'. (ob-shell/error-output-after-failure-multiple): On pass, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 9ba09908b..fb58e70e1 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -344,21 +344,23 @@ exit code, after exiting with a non-zero code." (buffer-string))))) (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest ob-shell/error-output-after-failure-multiple () +(ert-deftest test-ob-shell/error-output-after-failure-multiple () "Test that multiple standard error strings show in the error buffer, alongside multiple exit codes." - (should - (string= "1 + (if + (should + (string= "1 [ Babel evaluation exited with code 2 ] 3 [ Babel evaluation exited with code 4 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2; exit 2" nil) - (org-babel-execute:sh - "echo 3 >&2; exit 4" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2; exit 2" nil) + (org-babel-execute:sh + "echo 3 >&2; exit 4" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) ;;; Exit codes -- 2.38.1 [-- Attachment #18: 0017-test-ob-shell.el-Refactor-ob-shell-exit-code.patch --] [-- Type: application/octet-stream, Size: 1757 bytes --] From a02faa555d218ff6c042eb6590b106deedd77ecb Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:57:12 -0500 Subject: [PATCH 17/20] test-ob-shell.el: Refactor ob-shell/exit-code * test-ob-shell.el (ob-shell/exit-code): Rename `ob-shell/exit-code' to `test-ob-shell/exit-code'. (ob-shell/exit-code): On pass, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index fb58e70e1..be9d1488d 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -364,16 +364,18 @@ buffer, alongside multiple exit codes." ;;; Exit codes -(ert-deftest ob-shell/exit-code () +(ert-deftest test-ob-shell/exit-code () "Test that the exit code shows in the error buffer after exiting with a non-zero return code." - (should - (string= "[ Babel evaluation exited with code 1 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "exit 1" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (if + (should + (string= "[ Babel evaluation exited with code 1 ]" + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "exit 1" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) (ert-deftest ob-shell/exit-code-multiple () "Test that multiple exit codes show in the error buffer after -- 2.38.1 [-- Attachment #19: 0018-test-ob-shell.el-Refactor-ob-shell-exit-code-multipl.patch --] [-- Type: application/octet-stream, Size: 2033 bytes --] From b879fceda8e211bf4a0930474f36a8e48d935191 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 23 Dec 2022 22:58:50 -0500 Subject: [PATCH 18/20] test-ob-shell.el: Refactor ob-shell/exit-code-multiple * test-ob-shell.el (ob-shell/exit-code-multiple): Rename `ob-shell/exit-code-multiple' to `test-ob-shell/exit-code-multiple'. (ob-shell/exit-code-multiple): On pass, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index be9d1488d..e4a9849c6 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -377,19 +377,21 @@ with a non-zero return code." (buffer-string))))) (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest ob-shell/exit-code-multiple () +(ert-deftest test-ob-shell/exit-code-multiple () "Test that multiple exit codes show in the error buffer after exiting with a non-zero return code multiple times." - (should - (string= "[ Babel evaluation exited with code 1 ] + (if + (should + (string= "[ Babel evaluation exited with code 1 ] [ Babel evaluation exited with code 2 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "exit 1" nil) - (org-babel-execute:sh - "exit 2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "exit 1" nil) + (org-babel-execute:sh + "exit 2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) (provide 'test-ob-shell) -- 2.38.1 [-- Attachment #20: 0019-ob-shell-test.org-Remove-ob-shell-test.org.patch --] [-- Type: application/octet-stream, Size: 3138 bytes --] From 07a74181f5cf0f90eaea3abd296a5cf7254b3454 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Sat, 24 Dec 2022 11:29:06 -0500 Subject: [PATCH 19/20] ob-shell-test.org: Remove ob-shell-test.org * testing/examples/ob-shell-test.org: Delete file File was used by test-ob-shell.el to test ob-shell.el. Tests have been updated to use temporary buffers, rendering ob-shell-test.org unnecessary. --- testing/examples/ob-shell-test.org | 88 ------------------------------ 1 file changed, 88 deletions(-) delete mode 100644 testing/examples/ob-shell-test.org diff --git a/testing/examples/ob-shell-test.org b/testing/examples/ob-shell-test.org deleted file mode 100644 index 2510f4f96..000000000 --- a/testing/examples/ob-shell-test.org +++ /dev/null @@ -1,88 +0,0 @@ -#+Title: a collection of examples for ob-shell tests -#+OPTIONS: ^:nil - -* Sample data structures -#+NAME: sample_array -| one | -| two | -| three | - -#+NAME: sample_mapping_table -| first | one | -| second | two | -| third | three | - -#+NAME: sample_big_table -| bread | 2 | kg | -| spaghetti | 20 | cm | -| milk | 50 | dl | - -* Array tests - :PROPERTIES: - :ID: 0ba56632-8dc1-405c-a083-c204bae477cf - :END: -** Generic shell: no arrays -#+begin_src sh :exports results :results output :var array=sample_array -echo ${array} -#+end_src - -#+RESULTS: -: one two three - -** Bash shell: support for arrays -Bash will see a simple indexed array. In this test, we check that the -returned value is indeed only the first item of the array, as opposed to -the generic serialiation that will return all elements of the array as -a single string. -#+begin_src bash :exports results :results output :var array=sample_array -echo ${array} -#+end_src - -#+RESULTS: -: one - -* Associative array tests (simple map) - :PROPERTIES: - :ID: bec1a5b0-4619-4450-a8c0-2a746b44bf8d - :END: -** Generic shell: no special handing -The shell will see all values as a single string. -#+begin_src sh :exports results :results output :var table=sample_mapping_table -echo ${table} -#+end_src - -#+RESULTS: -: first one second two third three - -** Bash shell: support for associative arrays -Bash will see a table that contains the first column as the 'index' -of the associative array, and the second column as the value. -#+begin_src bash :exports :results output results :var table=sample_mapping_table -echo ${table[second]} -#+end_src - -#+RESULTS: -: two - -* Associative array tests (more than 2 columns) - :PROPERTIES: - :ID: 82320a48-3409-49d7-85c9-5de1c6d3ff87 - :END: -** Generic shell: no special handing -#+begin_src sh :exports results :results output :var table=sample_big_table -echo ${table} -#+end_src - -#+RESULTS: -: bread 2 kg spaghetti 20 cm milk 50 dl - -** Bash shell: support for associative arrays with lists -Bash will see an associative array that contains each row as a single -string. Bash cannot handle lists in associative arrays. -#+begin_src bash :exports results :results output :var table=sample_big_table -echo ${table[spaghetti]} -#+end_src - -#+RESULTS: -: 20 cm - -- 2.38.1 [-- Attachment #21: 0020-test-ob-shell.el-Organize-tests.patch --] [-- Type: application/octet-stream, Size: 2423 bytes --] From 9f352bbbfc4b49d3acd690328a8e43ba134baa83 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Sun, 25 Dec 2022 23:25:21 -0500 Subject: [PATCH 20/20] test-ob-shell.el: Organize tests * testing/lisp/test-ob-shell.el: - Provide instructions for running tests - Require `org-test' explicitly - Group tests into sections - Insert `page-delimiter's between sections --- testing/lisp/test-ob-shell.el | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index e4a9849c6..08bf40553 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -18,17 +18,26 @@ ;; You should have received a copy of the GNU General Public License ;; along with this program. If not, see <https://www.gnu.org/licenses/>. +\f ;;; Comment: +;; To run tests, load this file and then call (ert "test-ob-shell"). +;; See README for other ways to run tests. + +\f ;;; Requirements: -(org-test-for-executable "sh") +(require 'org-test (expand-file-name "../org-test.el")) (require 'ob-core) (unless (featurep 'ob-shell) (signal 'missing-test-dependency "Support for Shell code blocks")) +(org-test-for-executable "sh") + +\f ;;; Code: + (defun test-ob-shell-multiline-string (&rest strings) "Join STRINGS with newlines. @@ -43,6 +52,9 @@ Each expression of STRINGS should evaluate to a string. \(fn STRINGS)" (string-join strings "\n")) +\f +;;; Results + (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () "Expanded shell bodies should not start with a blank line unless the strings of the tangled block does." @@ -298,6 +310,7 @@ string. Bash cannot handle lists in associative arrays." "- 1\n- 2\n- 3\n" (buffer-substring-no-properties (point) (point-max)))))) +\f ;;; Standard output (ert-deftest test-ob-shell/standard-output-after-success () @@ -314,6 +327,7 @@ string. Bash cannot handle lists in associative arrays." "echo 1; exit 2" nil))) (kill-buffer "*Org-Babel Error Output*"))) +\f ;;; Standard error (ert-deftest test-ob-shell/error-output-after-success () @@ -362,6 +376,7 @@ buffer, alongside multiple exit codes." (buffer-string))))) (kill-buffer "*Org-Babel Error Output*"))) +\f ;;; Exit codes (ert-deftest test-ob-shell/exit-code () -- 2.38.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-27 20:48 ` Matt @ 2022-12-29 11:08 ` Ihor Radchenko 2022-12-29 14:20 ` Bastien Guerry 2022-12-30 5:34 ` Matt 0 siblings, 2 replies; 41+ messages in thread From: Ihor Radchenko @ 2022-12-29 11:08 UTC (permalink / raw) To: Matt, Bastien; +Cc: emacs-orgmode Bastien, could you please check Matt's copyright paperwork record in FSF? Matt <matt@excalamus.com> writes: > ---- On Wed, 21 Dec 2022 01:17:50 -0500 Matt wrote --- > > > Currently, though, I'm refactoring the ob-shell tests to remove dependency on ob-shell-test.org and to stop the suite from littering. > > Done. Branched off bugfix, 12e10eb0d, and refactored test-ob-shell.el. See attached patches. Better use main for development. bugfix is for... bugfixes :) > > After that, I intend to incorporate the worg examples as tests. > > This is probably a good place for me to pause and get feedback before writing more code. Sure. See below. > I just got the signed paperwork back from Craig and am waiting to be admitted to the Emacs group on Savannah. Does it mean that you are willing to maintain lisp/ob-shell.el? We usually give write access to the maintainers and regular contributors. AFAIR, you previously contributed to WORG but not to Org core. In order for you to get admitted to Emacs group, we will first need to reach out to Emacs maintainers authorizing your write access. Meanwhile, once Bastien confirms your copyright status, I can apply patches on your behalf. > From ecdb71afa00ca137b4faa737393cb027907a7f9f Mon Sep 17 00:00:00 2001 > From: Matt Trzcinski <matt@excalamus.com> > Date: Tue, 20 Dec 2022 13:51:26 -0500 > Subject: [PATCH 01/20] test-ob-shell.el: Remove mixed tabs and spaces > > * test-ob-shell.el: Convert tabs to spaces. > > Change made under the premise that one or the other is better than > both. We generally avoid whitespace-only changes. They tend to generate merge conflicts. We do whitespace changes alongside with other changes in the code only - this minimizes merge conflicts. See https://orgmode.org/list/874jvkz5k8.fsf@bzg.fr > (ert-deftest test-ob-shell/dont-error-on-empty-results () > - "Was throwing an elisp error when shell blocks threw errors and > -returned empty results." > - (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil)))) > + (should (null (org-babel-execute:sh nil nil)))) nil BODY argument for `org-babel-execute:sh' is not supported. Better use explicit empty body: "". It is also a good idea to add docstrings to the tests for completeness. > [fn:1] Session name is a string and not a variable (such as > `session-name'). This is because `org-babel-execute:sh' produces a > type error when the session name is defined with a variable. The > source of the error appears to be the `params' symbol in > `org-babel-execute:sh'. `params' does not evaluate as a variable as > expected–it evaluates as a symbol. However, `org-babel-execute:sh' is > defined within a function factory which makes it difficult to debug. > Hard code the test name for now and refactor it later once > `org-babel-execute:sh` is refactored. There is nothing wrong here. `org-babel-execute-src-block' takes care about parameter processing making sure that :session value is always a string. > +;; TODO refactor into macro. Currently violates (elisp) Coding > +;; Conventions and is hard to debug. > (defun org-babel-shell-initialize () > "Define execution functions associated to shell names. Could you please elaborate? Which particular convention does it violate? What is hard to debug? > (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () > "Expanded shell bodies should not start with a blank line unless > -the body of the tangled block does." > +the strings of the tangled block does." What does "strings of the tangled block" refer to? The previous variant is a lot more clear for me. > +(ert-deftest test-ob-shell/generic-uses-no-arrays () > + "Test generic serialization of array into a single string." > + (org-test-with-temp-text > + (test-ob-shell-multiline-string > + "#+NAME: sample_array" > + "| one |" > + "| two |" > + "| three |" Why do you need `test-ob-shell-multiline-string' here? Can simply type-in the string directly, as the rest of tests do. > +(require 'org-test (expand-file-name "../org-test.el")) I am unsure here. What will happen if you run this file from default-directory not the same with file location? Also, the repetitive patches changing names + killing error buffer for individual tests can be merged into a single patch. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-29 11:08 ` Ihor Radchenko @ 2022-12-29 14:20 ` Bastien Guerry 2022-12-30 5:34 ` Matt 1 sibling, 0 replies; 41+ messages in thread From: Bastien Guerry @ 2022-12-29 14:20 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Matt, emacs-orgmode Ihor Radchenko <yantar92@posteo.net> writes: > Bastien, could you please check Matt's copyright paperwork record in > FSF? Matt's copyright paperwork are OK, I added him as FSF-copyrighted contributor on Worg. > Does it mean that you are willing to maintain lisp/ob-shell.el? Until Matt wants to be the maintainer for ob-shell.el, I suggest Ihor applies the patches. > In order for you to get admitted to Emacs group, we will first need to > reach out to Emacs maintainers authorizing your write access. Matt, you reached out to the Emacs maintainers and they asked me whether I should give you write access: I told them to wait until you confirm you want to the be the ob-shell.el maintainer. If you don't, I think sharing patches that another Org maintainer applies is enough. Thanks! -- Bastien ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-29 11:08 ` Ihor Radchenko 2022-12-29 14:20 ` Bastien Guerry @ 2022-12-30 5:34 ` Matt 2022-12-30 8:06 ` Bastien Guerry ` (2 more replies) 1 sibling, 3 replies; 41+ messages in thread From: Matt @ 2022-12-30 5:34 UTC (permalink / raw) To: Ihor Radchenko; +Cc: bastien, emacs-orgmode ---- On Thu, 29 Dec 2022 06:08:59 -0500 Ihor Radchenko wrote --- > Does it mean that you are willing to maintain lisp/ob-shell.el? > We usually give write access to the maintainers and regular > contributors. AFAIR, you previously contributed to WORG but not to Org > core. You're correct, I've not contributed to core. I would love to maintain lisp/ob-shell.el. I'm expecting life changes in the coming months and can't anticipate how that will affect my time. Would it be a problem if I need to step down as maintainer for a period? > > From: Matt Trzcinski matt@excalamus.com> > > > > [fn:1] Session name is a string and not a variable (such as > > `session-name'). This is because `org-babel-execute:sh' produces a > > type error when the session name is defined with a variable. The > > source of the error appears to be the `params' symbol in > > `org-babel-execute:sh'. `params' does not evaluate as a variable as > > expected–it evaluates as a symbol. However, `org-babel-execute:sh' is > > defined within a function factory which makes it difficult to debug. > > Hard code the test name for now and refactor it later once > > `org-babel-execute:sh` is refactored. > > There is nothing wrong here. `org-babel-execute-src-block' takes care > about parameter processing making sure that :session value is always a > string. Originally the test used "yes" for a comparison string and the shell it ran in. I changed "yes" to be the test name because when it was "yes", I didn't know what test had produced it. However, the problem with the test name being hard coded as a string is that if the function name changes, the test string may get out of sync. More on this below. > > +;; TODO refactor into macro. Currently violates (elisp) Coding > > +;; Conventions and is hard to debug. > > (defun org-babel-shell-initialize () > > "Define execution functions associated to shell names. > > Could you please elaborate? Which particular convention does it violate? > What is hard to debug? (elisp) Coding Conventions says, "• Constructs that define a function or variable should be macros, not functions, and their names should start with ‘define-’. The macro should receive the name to be defined as the first argument. That will help various tools find the definition automatically. Avoid constructing the names in the macro itself, since that would confuse these tools." The `org-babel-shell-initialize' function defines *all* the `org-babel-execute:XXX' functions given by `org-babel-shell-names' (sh, bash, zsh, etc.). Because `org-babel-shell-initialize' is a function factory, you can't easily examine or modify their definitions. `C-h f org-babel-execute:sh' jumps to the top of lisp/ob-shell.el. Changing the definition requires reevaluating the definition for all the execute functions (or first changing `org-babel-shell-names'). This was a problem for me when I wanted to make the session name string for `test-ob-shell/session' the test name (mentioned above). In the test, when I replaced the session name string with a variable containing the string, `org-babel-execute:sh' failed with a type error. I couldn't get the variable to evaluate (with backquote and comma or otherwise). Without an explicit function definition or a macro to expand, I found it hard to debug/experiment with (and so left the test name as a hard coded string). > > (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () > > "Expanded shell bodies should not start with a blank line unless > > -the body of the tangled block does." > > +the strings of the tangled block does." > > What does "strings of the tangled block" refer to? The previous variant > is a lot more clear for me. I believe this is a query-replace error. Good catch! > > +(ert-deftest test-ob-shell/generic-uses-no-arrays () > > + "Test generic serialization of array into a single string." > > + (org-test-with-temp-text > > + (test-ob-shell-multiline-string > > + "#+NAME: sample_array" > > + "| one |" > > + "| two |" > > + "| three |" > > Why do you need `test-ob-shell-multiline-string' here? > Can simply type-in the string directly, as the rest of tests do. I probably don't need it and am happy to remove it. An older version of the function was more complex and made sense as a separate function (or so I thought). My aim was to make the test strings easy to read so that it's clearer what's being tested (i.e. not write multi-line strings on a single line). I could use concat and add "\n" to the end of each line. Or, simply write out the string-join. Maybe there's another way to write multi-line strings that I'm not aware of in Elisp, maybe something like Python's triple-quote? > > +(require 'org-test (expand-file-name "../org-test.el")) > > I am unsure here. What will happen if you run this file from > default-directory not the same with file location? I'm unsure if this is the best approach. AFAICT, it works. For example, (let ((default-directory "/home/ahab/Documents/")) (load "/home/ahab/Projects/org-mode/testing/lisp/test-ob-shell.el")) The problem I'm trying to solve is that the test file relies on `org-test' yet doesn't declare it. Instead, you need to know to load it ahead of time (or run into the error when trying to load the tests). > Also, the repetitive patches changing names + killing error buffer for > individual tests can be merged into a single patch. Thank you for taking the time to review everything. I'll get a new patch set created that includes updates based on all your feedback. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-30 5:34 ` Matt @ 2022-12-30 8:06 ` Bastien Guerry 2022-12-30 18:46 ` Matt 2022-12-31 12:56 ` ob-shell intentions and paperwork (was Bash results broken?) Ihor Radchenko 2 siblings, 0 replies; 41+ messages in thread From: Bastien Guerry @ 2022-12-30 8:06 UTC (permalink / raw) To: Matt; +Cc: Ihor Radchenko, emacs-orgmode Hi Matthew, Matt <matt@excalamus.com> writes: > You're correct, I've not contributed to core. I would love to > maintain lisp/ob-shell.el. Your wish has been granted: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e8ceb4a2 > I'm expecting life changes in the coming > months and can't anticipate how that will affect my time. Would it be > a problem if I need to step down as maintainer for a period? It is no problem at all. You don't even need to step down as maintainer: if requests against ob-shell.el are not *that* pressing during the time you're not fully available, users and maintainers can just be patient. I've warned Emacs maintainers and they'll give you write access to the Org repository. Thanks! -- Bastien ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-30 5:34 ` Matt 2022-12-30 8:06 ` Bastien Guerry @ 2022-12-30 18:46 ` Matt 2022-12-31 14:31 ` Ihor Radchenko 2022-12-31 12:56 ` ob-shell intentions and paperwork (was Bash results broken?) Ihor Radchenko 2 siblings, 1 reply; 41+ messages in thread From: Matt @ 2022-12-30 18:46 UTC (permalink / raw) To: ihor radchenko; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 1025 bytes --] ---- On Fri, 30 Dec 2022 00:34:38 -0500 Matt wrote --- > > ---- On Thu, 29 Dec 2022 06:08:59 -0500 Ihor Radchenko wrote --- > > > From: Matt Trzcinski matt@excalamus.com> > > > +(require 'org-test (expand-file-name "../org-test.el")) > > > > I am unsure here. What will happen if you run this file from > > default-directory not the same with file location? I was mistaken. Loading a new Emacs instance and running `(load "/path/to/test-ob-shell.el")' fails with file-missing error. I'm content to leave that alone for now. I've backed out the `require' change and adjusted everything else based on your feedback. There is a separate patch for each refactor that created a new test. The remaining refactors are in a single patch. I was also able to resolve the issue I had with inserting the test name for the session (the "yes" string in `test-ob-shell/session'). The issue with `org-babel-shell-initialize' still stands but is outside the scope of these patches and isn't dealt with here. [-- Attachment #2: 0001-test-ob-shell.el-Split-test-ob-shell-dont-error-on-e.patch --] [-- Type: application/octet-stream, Size: 1899 bytes --] From e204c3a6ccdaf1770db5e931f1116235597a8c0b Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 11:18:15 -0500 Subject: [PATCH 1/7] test-ob-shell.el: Split test-ob-shell/dont-error-on-empty-results * testing/lisp/test-ob-shell.el (test-ob-shell/dont-error-on-empty-results): Explicitly test handling of empty results. (test-ob-shell/dont-error-on-babel-error): Explicitly test handling of Babel errors. On pass, remove buffers created during test. Original test conflated empty results (a valid Babel return) and Babel errors. --- testing/lisp/test-ob-shell.el | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index b0d9beff4..fc3ecd3c8 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -34,12 +34,16 @@ unless the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "\n\necho 2" '())))) + (org-babel-expand-body:generic "\n\necho 2" '())))) (ert-deftest test-ob-shell/dont-error-on-empty-results () - "Was throwing an elisp error when shell blocks threw errors and -returned empty results." - (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil)))) + "Empty results should not cause a Lisp error." + (should (null (org-babel-execute:sh "" nil)))) + +(ert-deftest test-ob-shell/dont-error-on-babel-error () + "Errors within Babel execution should not cause Lisp errors." + (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) + (kill-buffer "*Org-Babel Error Output*"))) (ert-deftest test-ob-shell/session () "This also tests `org-babel-comint-with-output' in -- 2.38.1 [-- Attachment #3: 0002-test-ob-shell.el-Refactor-test-ob-shell-session.patch --] [-- Type: application/octet-stream, Size: 2607 bytes --] From 6e0012867d8594913cf4783d7afdc109b8307de3 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 11:35:10 -0500 Subject: [PATCH 2/7] test-ob-shell.el: Refactor test-ob-shell/session * testing/lisp/test-ob-shell.el (ob-shell/session): Split `ob-shell/session' into `test-ob-shell/session-single-return-returns-string' and `test-ob-shell/session-multiple-returns-returns-list'. (ob-shell/session): Rename comint from to test name. On pass, kill process and remove process buffer. --- testing/lisp/test-ob-shell.el | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index fc3ecd3c8..b08bf8413 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -45,18 +45,28 @@ unless the body of the tangled block does." (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest test-ob-shell/session () - "This also tests `org-babel-comint-with-output' in -ob-comint.el, which was not previously tested." - (let ((res (org-babel-execute:sh "echo 1; echo 2" '((:session . "yes"))))) - (should res) - (should (listp res))) - ;; Test multi-line input. - (let ((result (org-babel-execute:sh - "if true \n then \n echo yes \n fi" - '((:session . "yes"))))) +(ert-deftest test-ob-shell/session-single-return-returns-string () + "Sessions with a single result should return a string." + (let* ((session-name "test-ob-shell/session-evaluation-single-return-returns-string") + (kill-buffer-query-functions nil) + (result (org-babel-execute:sh + (format "echo %s" session-name) + `((:session . ,session-name))))) (should result) - (should (string= "yes" result)))) + (if (should (string= session-name result)) + (kill-buffer session-name)))) + +(ert-deftest test-ob-shell/session-multiple-returns-returns-list () + "Sessions with multiple results should return a list." + (let* ((session-name "test-ob-shell/session-multiple-returns-returns-list") + (kill-buffer-query-functions nil) + (result (org-babel-execute:sh + "echo 1; echo 2" + `((:session . ,session-name))))) + (should result) + (should (listp result)) + (if (should (equal '((1) (2)) result)) + (kill-buffer session-name)))) ; A list of tests using the samples in ob-shell-test.org (ert-deftest ob-shell/generic-uses-no-arrays () -- 2.38.1 [-- Attachment #4: 0003-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch --] [-- Type: application/octet-stream, Size: 3522 bytes --] From d44204d940b81c284102981291433180ca9a438f Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 11:39:35 -0500 Subject: [PATCH 3/7] test-ob-shell.el: Refactor ob-shell/generic-uses-no-assoc-arrays * test-ob-shell.el (ob-shell/generic-uses-no-assoc-arrays): Split test based on simple versus complex mapping cases, `test-ob-shell/generic-uses-no-assoc-arrays-simple-map' and `test-ob-shell/generic-uses-no-assoc-arrays-3-columns'. Rename prefix from "ob-shell" to "test-ob-shell". (ob-shell/generic-uses-no-assoc-arrays): Move headlines and commentary from `testing/examples/ob-shell-test.org' to docstring. (ob-shell/generic-uses-no-assoc-arrays): Remove dependency on testing/examples/ob-shell-test.org. --- testing/lisp/test-ob-shell.el | 56 +++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index b08bf8413..57347229e 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -68,7 +68,6 @@ unless the body of the tangled block does." (if (should (equal '((1) (2)) result)) (kill-buffer session-name)))) -; A list of tests using the samples in ob-shell-test.org (ert-deftest ob-shell/generic-uses-no-arrays () "No arrays for generic" (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" @@ -81,20 +80,47 @@ unless the body of the tangled block does." (org-babel-next-src-block 2) (should (equal "one" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/generic-uses-no-assoc-arrays () - "No associative arrays for generic" - (should - (equal "first one second two third three" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block))))) - (should - (equal "bread 2 kg spaghetti 20 cm milk 50 dl" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block)))))) +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-simple-map () + "Generic shell: no special handing for key-value mapping table + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + (string-join + '("#+NAME: sample_mapping_table" + "| first | one |" + "| second | two |" + "| third | three |" + "" + "#+begin_src sh :exports results :results output :var table=sample_mapping_table" + "echo ${table}" + "<point>" + "#+end_src") + "\n") + (should + (equal "first one second two third three" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-3-columns () + "Associative array tests (more than 2 columns) + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + (string-join + '("#+NAME: sample_big_table" + "| bread | 2 | kg |" + "| spaghetti | 20 | cm |" + "| milk | 50 | dl |" + "" + "#+begin_src sh :exports results :results output :var table=sample_big_table" + "echo ${table}" + "<point>" + "#+end_src") + "\n") + (should + (equal "bread 2 kg spaghetti 20 cm milk 50 dl" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-assoc-arrays () "Bash associative arrays" -- 2.38.1 [-- Attachment #5: 0004-Refactor-ob-shell-bash-uses-assoc-arrays.patch --] [-- Type: application/octet-stream, Size: 3295 bytes --] From 8a9f5b29ec6bf09f63bc41dab09781b3456ff2aa Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 12:36:41 -0500 Subject: [PATCH 4/7] Refactor ob-shell/bash-uses-assoc-arrays * testing/lisp/test-ob-shell.el (ob-shell/bash-uses-assoc-arrays): Split test cases into separate tests, `test-ob-shell/bash-uses-assoc-arrays' and `test-ob-shell/bash-uses-assoc-arrays-with-lists'. Rename prefix from "ob-shell" to "test-ob-shell". (ob-shell/bash-uses-assoc-arrays): Move comments from testing/examples/ob-shell-test.org to docstrings. (ob-shell/bash-uses-assoc-arrays): Remove dependency on testing/examples/ob-shell-test.org. --- testing/lisp/test-ob-shell.el | 61 +++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 17 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 57347229e..1e4bd6729 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -119,24 +119,51 @@ as a single string." "#+end_src") "\n") (should - (equal "bread 2 kg spaghetti 20 cm milk 50 dl" - (org-trim (org-babel-execute-src-block)))))) + (equal "bread 2 kg spaghetti 20 cm milk 50 dl" + (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-assoc-arrays () - "Bash associative arrays" - (should - (equal "two" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block))))) - ;; Bash associative arrays as strings for the row. - (should - (equal "20 cm" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block)))))) +(ert-deftest test-ob-shell/bash-uses-assoc-arrays () + "Bash shell: support for associative arrays + +Bash will see a table that contains the first column as the +'index' of the associative array, and the second column as the +value. " + (org-test-with-temp-text + (string-join + '("#+NAME: sample_mapping_table" + "| first | one |" + "| second | two |" + "| third | three |" + "" + "#+begin_src bash :exports :results output results :var table=sample_mapping_table" + "echo ${table[second]}" + "<point>" + "#+end_src") + "\n") + (should + (equal "two" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/bash-uses-assoc-arrays-with-lists () + "Bash shell: support for associative arrays with lists + +Bash will see an associative array that contains each row as a single +string. Bash cannot handle lists in associative arrays." + (org-test-with-temp-text + (string-join + '("#+NAME: sample_big_table" + "| bread | 2 | kg |" + "| spaghetti | 20 | cm |" + "| milk | 50 | dl |" + "" + "#+begin_src bash :exports results :results output :var table=sample_big_table" + "echo ${table[spaghetti]}" + "<point>" + "#+end_src") + "\n") + (should + (equal "20 cm" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/simple-list () "Test list variables in shell." -- 2.38.1 [-- Attachment #6: 0005-test-ob-shell.el-Refactor-test-names-and-kill-test-b.patch --] [-- Type: application/octet-stream, Size: 15159 bytes --] From fbdb368efdbedc5b8f5ed6b27f74be060a4c4cb8 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 12:39:47 -0500 Subject: [PATCH 5/7] test-ob-shell.el: Refactor test names and kill test buffers * testing/lisp/test-ob-shell.el: (ob-shell/generic-uses-no-arrays): Rename test from `ob-shell/generic-uses-no-arrays' to `test-ob-shell/generic-uses-no-arrays'. (ob-shell/generic-uses-no-arrays): Move comments from testing/examples/ob-shell-test.org to docstring. (ob-shell/generic-uses-no-arrays): Remove dependency on testing/examples/ob-shell-test.org. (ob-shell/bash-uses-arrays): Rename `ob-shell/bash-uses-arrays' to `test-ob-shell/bash-uses-arrays'. (ob-shell/bash-uses-arrays): Move comments from testing/examples/ob-shell-test.org to docstring. (ob-shell/bash-uses-arrays): Remove dependency on testing/examples/ob-shell-test.org. (ob-shell/simple-list): Change test name from `ob-shell/simple-list' to `test-ob-shell/simple-list'. (ob-shell/remote-with-stdin-or-cmdline): Change test name from `ob-shell/remote-with-stdin-or-cmdline' to `test-ob-shell/remote-with-stdin-or-cmdline'. (ob-shell/remote-with-stdin-or-cmdline): On pass, kill buffer created by test. (ob-shell/results-table): Rename `ob-shell/results-table' to `test-ob-shell/results-table'. (ob-shell/results-list): Rename `ob-shell/results-list' to `test-ob-shell/results-list'. (ob-shell/standard-output-after-success): Rename `ob-shell/standard-output-after-success' to `test-ob-shell/standard-output-after-success'. (ob-shell/standard-output-after-failure): Rename `ob-shell/standard-output-after-failure' to `test-ob-shell/standard-output-after-failure'. (ob-shell/standard-output-after-failure): On pass, kill buffer created during test. (ob-shell/error-output-after-success): Rename `ob-shell/error-output-after-success' to `test-ob-shell/error-output-after-success'. (ob-shell/error-output-after-success): On pass, kill buffer created by test. (ob-shell/error-output-after-failure): Rename `ob-shell/error-output-after-failure' to `test-ob-shell/error-output-after-failure'. (ob-shell/error-output-after-failure): On pass, kill buffer created by test. (ob-shell/error-output-after-failure-multiple): Rename `ob-shell/error-output-after-failure-multiple' to `test-ob-shell/error-output-after-failure-multiple'. (ob-shell/error-output-after-failure-multiple): On pass, kill buffer created by test. (ob-shell/exit-code): Rename `ob-shell/exit-code' to `test-ob-shell/exit-code'. (ob-shell/exit-code): On pass, kill buffer created by test. (ob-shell/exit-code-multiple): Rename `ob-shell/exit-code-multiple' to `test-ob-shell/exit-code-multiple'. (ob-shell/exit-code-multiple): On pass, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 234 +++++++++++++++++++++------------- 1 file changed, 146 insertions(+), 88 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 1e4bd6729..2c4491ffa 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -18,21 +18,23 @@ ;; You should have received a copy of the GNU General Public License ;; along with this program. If not, see <https://www.gnu.org/licenses/>. -;;; Comment: -;; Template test file for Org tests +;;; Comment: -;;; Code: +;;; Requirements: (org-test-for-executable "sh") + (require 'ob-core) + (unless (featurep 'ob-shell) (signal 'missing-test-dependency "Support for Shell code blocks")) +;;; Code: (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () - "Expanded shell bodies should not start with a blank line -unless the body of the tangled block does." + "Expanded shell bodies should not start with a blank line unless +the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "echo 2" '()))) + (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" (org-babel-expand-body:generic "\n\necho 2" '())))) @@ -68,16 +70,40 @@ unless the body of the tangled block does." (if (should (equal '((1) (2)) result)) (kill-buffer session-name)))) -(ert-deftest ob-shell/generic-uses-no-arrays () - "No arrays for generic" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block) +(ert-deftest test-ob-shell/generic-uses-no-arrays () + "Test generic serialization of array into a single string." + (org-test-with-temp-text + (string-join + '("#+NAME: sample_array" + "| one |" + "| two |" + "| three |" + "" + "#+begin_src sh :exports results :results output :var array=sample_array" + "echo ${array}" + "<point>" + "#+end_src") + "\n") (should (equal "one two three" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-arrays () - "Bash arrays" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block 2) +(ert-deftest test-ob-shell/bash-uses-arrays () + "Bash sees named array as a simple indexed array. + +In this test, we check that the returned value is indeed only the +first item of the array, as opposed to the generic serialiation +that will return all elements of the array as a single string." + (org-test-with-temp-text + (string-join + '("#+NAME: sample_array" + "| one |" + "| two |" + "| three |" + "" + "#+begin_src bash :exports results :results output :var array=sample_array" + "echo ${array}" + "<point>" + "#+end_src") + "\n") (should (equal "one" (org-trim (org-babel-execute-src-block)))))) (ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-simple-map () @@ -165,22 +191,31 @@ string. Bash cannot handle lists in associative arrays." (equal "20 cm" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/simple-list () - "Test list variables in shell." - ;; With bash, a list is turned into an array. +(ert-deftest test-ob-shell/simple-list () + "Test list variables." + ;; bash: a list is turned into an array (should (equal "2" - (org-test-with-temp-text - "#+BEGIN_SRC bash :results output :var l='(1 2)\necho ${l[1]}\n#+END_SRC" - (org-trim (org-babel-execute-src-block))))) - ;; On sh, it is a string containing all values. + (org-test-with-temp-text + (string-join + '("#+BEGIN_SRC bash :results output :var l='(1 2)" + "echo ${l[1]}" + "#+END_SRC") + "\n") + (org-trim (org-babel-execute-src-block))))) + + ;; sh: a list is a string containing all values (should (equal "1 2" - (org-test-with-temp-text - "#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC" - (org-trim (org-babel-execute-src-block)))))) + (org-test-with-temp-text + (string-join + '("#+BEGIN_SRC sh :results output :var l='(1 2)" + "echo ${l}" + "#+END_SRC") + "\n") + (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/remote-with-stdin-or-cmdline () +(ert-deftest test-ob-shell/remote-with-stdin-or-cmdline () "Test :stdin and :cmdline with a remote directory." ;; We assume `default-directory' is a local directory. (skip-unless (not (memq system-type '(ms-dos windows-nt)))) @@ -227,20 +262,31 @@ string. Bash cannot handle lists in associative arrays." (org-trim (org-babel-execute-src-block)))) (expected (concat "ARGS: --verbose 23 71" "\nhello tramp from " (file-local-name default-directory)))) - (should (equal result expected))))))) + (if (should (equal result expected)) + (kill-matching-buffers (format "\\*tramp/mock\\s-%s\\*" system-name) t t))))))) -(ert-deftest ob-shell/results-table () +(ert-deftest test-ob-shell/results-table () "Test :results table." (should (equal '(("I \"want\" it all")) - (org-test-with-temp-text - "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC" - (org-babel-execute-src-block))))) - -(ert-deftest ob-shell/results-list () + (org-test-with-temp-text + (string-join + '("#+BEGIN_SRC sh :results table" + "echo 'I \"want\" it all'" + "#+END_SRC") + "\n") + (org-babel-execute-src-block))))) + +(ert-deftest test-ob-shell/results-list () "Test :results list." (org-test-with-temp-text - "#+BEGIN_SRC sh :results list\necho 1\necho 2\necho 3\n#+END_SRC" + (string-join + '("#+BEGIN_SRC sh :results list" + "echo 1" + "echo 2" + "echo 3" + "#+END_SRC") + "\n") (should (equal '((1) (2) (3)) (org-babel-execute-src-block))) @@ -253,86 +299,98 @@ string. Bash cannot handle lists in associative arrays." ;;; Standard output -(ert-deftest ob-shell/standard-output-after-success () +(ert-deftest test-ob-shell/standard-output-after-success () "Test standard output after exiting with a zero code." (should (= 1 (org-babel-execute:sh "echo 1" nil)))) -(ert-deftest ob-shell/standard-output-after-failure () +(ert-deftest test-ob-shell/standard-output-after-failure () "Test standard output after exiting with a non-zero code." - (should (= 1 - (org-babel-execute:sh - "echo 1; exit 2" nil)))) + (if + (should (= 1 + (org-babel-execute:sh + "echo 1; exit 2" nil))) + (kill-buffer "*Org-Babel Error Output*"))) ;;; Standard error -(ert-deftest ob-shell/error-output-after-success () - "Test that standard error shows in the error buffer, alongside the -exit code, after exiting with a zero code." - (should - (string= "1 +(ert-deftest test-ob-shell/error-output-after-success () + "Test that standard error shows in the error buffer, alongside +the exit code, after exiting with a zero code." + (if + (should + (string= "1 [ Babel evaluation exited with code 0 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) - -(ert-deftest ob-shell/error-output-after-failure () - "Test that standard error shows in the error buffer, alongside the -exit code, after exiting with a non-zero code." - (should - (string= "1 + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) + +(ert-deftest test-ob-shell/error-output-after-failure () + "Test that standard error shows in the error buffer, alongside +the exit code, after exiting with a non-zero code." + (if + (should + (string= "1 [ Babel evaluation exited with code 2 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2; exit 2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2; exit 2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest ob-shell/error-output-after-failure-multiple () +(ert-deftest test-ob-shell/error-output-after-failure-multiple () "Test that multiple standard error strings show in the error buffer, alongside multiple exit codes." - (should - (string= "1 + (if + (should + (string= "1 [ Babel evaluation exited with code 2 ] 3 [ Babel evaluation exited with code 4 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2; exit 2" nil) - (org-babel-execute:sh - "echo 3 >&2; exit 4" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2; exit 2" nil) + (org-babel-execute:sh + "echo 3 >&2; exit 4" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) ;;; Exit codes -(ert-deftest ob-shell/exit-code () +(ert-deftest test-ob-shell/exit-code () "Test that the exit code shows in the error buffer after exiting with a non-zero return code." - (should - (string= "[ Babel evaluation exited with code 1 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "exit 1" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) - -(ert-deftest ob-shell/exit-code-multiple () + (if + (should + (string= "[ Babel evaluation exited with code 1 ]" + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "exit 1" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) + +(ert-deftest test-ob-shell/exit-code-multiple () "Test that multiple exit codes show in the error buffer after exiting with a non-zero return code multiple times." - (should - (string= "[ Babel evaluation exited with code 1 ] + (if + (should + (string= "[ Babel evaluation exited with code 1 ] [ Babel evaluation exited with code 2 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "exit 1" nil) - (org-babel-execute:sh - "exit 2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "exit 1" nil) + (org-babel-execute:sh + "exit 2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) (provide 'test-ob-shell) -- 2.38.1 [-- Attachment #7: 0006-ob-shell-test.org-Remove-ob-shell-test.org.patch --] [-- Type: application/octet-stream, Size: 3136 bytes --] From ec1653eed46254fba0596d494da9142fcea6da13 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Sat, 24 Dec 2022 11:29:06 -0500 Subject: [PATCH 6/7] ob-shell-test.org: Remove ob-shell-test.org * testing/examples/ob-shell-test.org: Delete file File was used by test-ob-shell.el to test ob-shell.el. Tests have been updated to use temporary buffers, rendering ob-shell-test.org unnecessary. --- testing/examples/ob-shell-test.org | 88 ------------------------------ 1 file changed, 88 deletions(-) delete mode 100644 testing/examples/ob-shell-test.org diff --git a/testing/examples/ob-shell-test.org b/testing/examples/ob-shell-test.org deleted file mode 100644 index 2510f4f96..000000000 --- a/testing/examples/ob-shell-test.org +++ /dev/null @@ -1,88 +0,0 @@ -#+Title: a collection of examples for ob-shell tests -#+OPTIONS: ^:nil - -* Sample data structures -#+NAME: sample_array -| one | -| two | -| three | - -#+NAME: sample_mapping_table -| first | one | -| second | two | -| third | three | - -#+NAME: sample_big_table -| bread | 2 | kg | -| spaghetti | 20 | cm | -| milk | 50 | dl | - -* Array tests - :PROPERTIES: - :ID: 0ba56632-8dc1-405c-a083-c204bae477cf - :END: -** Generic shell: no arrays -#+begin_src sh :exports results :results output :var array=sample_array -echo ${array} -#+end_src - -#+RESULTS: -: one two three - -** Bash shell: support for arrays -Bash will see a simple indexed array. In this test, we check that the -returned value is indeed only the first item of the array, as opposed to -the generic serialiation that will return all elements of the array as -a single string. -#+begin_src bash :exports results :results output :var array=sample_array -echo ${array} -#+end_src - -#+RESULTS: -: one - -* Associative array tests (simple map) - :PROPERTIES: - :ID: bec1a5b0-4619-4450-a8c0-2a746b44bf8d - :END: -** Generic shell: no special handing -The shell will see all values as a single string. -#+begin_src sh :exports results :results output :var table=sample_mapping_table -echo ${table} -#+end_src - -#+RESULTS: -: first one second two third three - -** Bash shell: support for associative arrays -Bash will see a table that contains the first column as the 'index' -of the associative array, and the second column as the value. -#+begin_src bash :exports :results output results :var table=sample_mapping_table -echo ${table[second]} -#+end_src - -#+RESULTS: -: two - -* Associative array tests (more than 2 columns) - :PROPERTIES: - :ID: 82320a48-3409-49d7-85c9-5de1c6d3ff87 - :END: -** Generic shell: no special handing -#+begin_src sh :exports results :results output :var table=sample_big_table -echo ${table} -#+end_src - -#+RESULTS: -: bread 2 kg spaghetti 20 cm milk 50 dl - -** Bash shell: support for associative arrays with lists -Bash will see an associative array that contains each row as a single -string. Bash cannot handle lists in associative arrays. -#+begin_src bash :exports results :results output :var table=sample_big_table -echo ${table[spaghetti]} -#+end_src - -#+RESULTS: -: 20 cm - -- 2.38.1 [-- Attachment #8: 0007-test-ob-shell.el-Organize-tests.patch --] [-- Type: application/octet-stream, Size: 2018 bytes --] From d50f940799fb5d6f2da5712a6423e498db3c530d Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 13:03:59 -0500 Subject: [PATCH 7/7] test-ob-shell.el: Organize tests * testing/lisp/test-ob-shell.el: - Give instructions on how to run tests - Require `org-test' explicitly - Group tests into sections - Insert `page-delimiter's between sections --- testing/lisp/test-ob-shell.el | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 2c4491ffa..d1244c9e4 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -18,17 +18,22 @@ ;; You should have received a copy of the GNU General Public License ;; along with this program. If not, see <https://www.gnu.org/licenses/>. - +\f ;;; Comment: +;; See testing/README for how to run tests. + +\f ;;; Requirements: -(org-test-for-executable "sh") (require 'ob-core) (unless (featurep 'ob-shell) (signal 'missing-test-dependency "Support for Shell code blocks")) +(org-test-for-executable "sh") + +\f ;;; Code: (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () "Expanded shell bodies should not start with a blank line unless @@ -297,6 +302,7 @@ string. Bash cannot handle lists in associative arrays." "- 1\n- 2\n- 3\n" (buffer-substring-no-properties (point) (point-max)))))) +\f ;;; Standard output (ert-deftest test-ob-shell/standard-output-after-success () @@ -313,6 +319,7 @@ string. Bash cannot handle lists in associative arrays." "echo 1; exit 2" nil))) (kill-buffer "*Org-Babel Error Output*"))) +\f ;;; Standard error (ert-deftest test-ob-shell/error-output-after-success () @@ -361,6 +368,7 @@ buffer, alongside multiple exit codes." (buffer-string))))) (kill-buffer "*Org-Babel Error Output*"))) +\f ;;; Exit codes (ert-deftest test-ob-shell/exit-code () -- 2.38.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-30 18:46 ` Matt @ 2022-12-31 14:31 ` Ihor Radchenko 2023-01-01 23:55 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2022-12-31 14:31 UTC (permalink / raw) To: Matt; +Cc: emacs-orgmode Matt <matt@excalamus.com> writes: > I've backed out the `require' change and adjusted everything else based on your feedback. There is a separate patch for each refactor that created a new test. The remaining refactors are in a single patch. I was also able to resolve the issue I had with inserting the test name for the session (the "yes" string in `test-ob-shell/session'). The issue with `org-babel-shell-initialize' still stands but is outside the scope of these patches and isn't dealt with here. Thanks! I can see that you still prefer to use `string-join' for Org body. Why not a bare string? There is no particular problem with `string-join', but the rest of the tests are all using direct string when possible - your patch makes ob-shell stand out of common style. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-31 14:31 ` Ihor Radchenko @ 2023-01-01 23:55 ` Matt 2023-01-02 9:47 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2023-01-01 23:55 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 1082 bytes --] ---- On Sat, 31 Dec 2022 09:31:16 -0500 Ihor Radchenko wrote --- > Matt matt@excalamus.com> writes: > > > I've backed out the `require' change and adjusted everything else based on your feedback. There is a separate patch for each refactor that created a new test. The remaining refactors are in a single patch. I was also able to resolve the issue I had with inserting the test name for the session (the "yes" string in `test-ob-shell/session'). > > Thanks! > > I can see that you still prefer to use `string-join' for Org body. > Why not a bare string? Because I often program in Python where double quoted strings typically don't span lines. One of the original tests had the string "#+BEGIN_SRC sh :results list\necho 1\necho 2\necho 3\n#+END_SRC" in it (that is, used newline characters instead of inserting new lines), so my brain assumed strings Emacs Lisp behaved similarly to Python. Silly brain :) Thanks for continuing to follow up on that. I've updated the patches to use bare strings. If this set of patches look good, I can push them to main. [-- Attachment #2: 0001-test-ob-shell.el-Split-test-ob-shell-dont-error-on-e.patch --] [-- Type: application/octet-stream, Size: 1899 bytes --] From e204c3a6ccdaf1770db5e931f1116235597a8c0b Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 11:18:15 -0500 Subject: [PATCH 1/7] test-ob-shell.el: Split test-ob-shell/dont-error-on-empty-results * testing/lisp/test-ob-shell.el (test-ob-shell/dont-error-on-empty-results): Explicitly test handling of empty results. (test-ob-shell/dont-error-on-babel-error): Explicitly test handling of Babel errors. On pass, remove buffers created during test. Original test conflated empty results (a valid Babel return) and Babel errors. --- testing/lisp/test-ob-shell.el | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index b0d9beff4..fc3ecd3c8 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -34,12 +34,16 @@ unless the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "\n\necho 2" '())))) + (org-babel-expand-body:generic "\n\necho 2" '())))) (ert-deftest test-ob-shell/dont-error-on-empty-results () - "Was throwing an elisp error when shell blocks threw errors and -returned empty results." - (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil)))) + "Empty results should not cause a Lisp error." + (should (null (org-babel-execute:sh "" nil)))) + +(ert-deftest test-ob-shell/dont-error-on-babel-error () + "Errors within Babel execution should not cause Lisp errors." + (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) + (kill-buffer "*Org-Babel Error Output*"))) (ert-deftest test-ob-shell/session () "This also tests `org-babel-comint-with-output' in -- 2.38.1 [-- Attachment #3: 0002-test-ob-shell.el-Refactor-test-ob-shell-session.patch --] [-- Type: application/octet-stream, Size: 2607 bytes --] From 6e0012867d8594913cf4783d7afdc109b8307de3 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 11:35:10 -0500 Subject: [PATCH 2/7] test-ob-shell.el: Refactor test-ob-shell/session * testing/lisp/test-ob-shell.el (ob-shell/session): Split `ob-shell/session' into `test-ob-shell/session-single-return-returns-string' and `test-ob-shell/session-multiple-returns-returns-list'. (ob-shell/session): Rename comint from to test name. On pass, kill process and remove process buffer. --- testing/lisp/test-ob-shell.el | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index fc3ecd3c8..b08bf8413 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -45,18 +45,28 @@ unless the body of the tangled block does." (if (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil))) (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest test-ob-shell/session () - "This also tests `org-babel-comint-with-output' in -ob-comint.el, which was not previously tested." - (let ((res (org-babel-execute:sh "echo 1; echo 2" '((:session . "yes"))))) - (should res) - (should (listp res))) - ;; Test multi-line input. - (let ((result (org-babel-execute:sh - "if true \n then \n echo yes \n fi" - '((:session . "yes"))))) +(ert-deftest test-ob-shell/session-single-return-returns-string () + "Sessions with a single result should return a string." + (let* ((session-name "test-ob-shell/session-evaluation-single-return-returns-string") + (kill-buffer-query-functions nil) + (result (org-babel-execute:sh + (format "echo %s" session-name) + `((:session . ,session-name))))) (should result) - (should (string= "yes" result)))) + (if (should (string= session-name result)) + (kill-buffer session-name)))) + +(ert-deftest test-ob-shell/session-multiple-returns-returns-list () + "Sessions with multiple results should return a list." + (let* ((session-name "test-ob-shell/session-multiple-returns-returns-list") + (kill-buffer-query-functions nil) + (result (org-babel-execute:sh + "echo 1; echo 2" + `((:session . ,session-name))))) + (should result) + (should (listp result)) + (if (should (equal '((1) (2)) result)) + (kill-buffer session-name)))) ; A list of tests using the samples in ob-shell-test.org (ert-deftest ob-shell/generic-uses-no-arrays () -- 2.38.1 [-- Attachment #4: 0003-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch --] [-- Type: application/octet-stream, Size: 3342 bytes --] From 728e7fca81201993a1a411980b4ec8210d607af9 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 11:39:35 -0500 Subject: [PATCH 3/7] test-ob-shell.el: Refactor ob-shell/generic-uses-no-assoc-arrays * test-ob-shell.el (ob-shell/generic-uses-no-assoc-arrays): Split test based on simple versus complex mapping cases, `test-ob-shell/generic-uses-no-assoc-arrays-simple-map' and `test-ob-shell/generic-uses-no-assoc-arrays-3-columns'. Rename prefix from "ob-shell" to "test-ob-shell". (ob-shell/generic-uses-no-assoc-arrays): Move headlines and commentary from `testing/examples/ob-shell-test.org' to docstring. (ob-shell/generic-uses-no-assoc-arrays): Remove dependency on testing/examples/ob-shell-test.org. --- testing/lisp/test-ob-shell.el | 52 +++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index b08bf8413..a39c99d9f 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -68,7 +68,6 @@ unless the body of the tangled block does." (if (should (equal '((1) (2)) result)) (kill-buffer session-name)))) -; A list of tests using the samples in ob-shell-test.org (ert-deftest ob-shell/generic-uses-no-arrays () "No arrays for generic" (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" @@ -81,20 +80,43 @@ unless the body of the tangled block does." (org-babel-next-src-block 2) (should (equal "one" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/generic-uses-no-assoc-arrays () - "No associative arrays for generic" - (should - (equal "first one second two third three" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block))))) - (should - (equal "bread 2 kg spaghetti 20 cm milk 50 dl" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block) - (org-trim (org-babel-execute-src-block)))))) +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-simple-map () + "Generic shell: no special handing for key-value mapping table + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + "#+NAME: sample_mapping_table +| first | one | +| second | two | +| third | three | + +#+begin_src sh :exports results :results output :var table=sample_mapping_table +echo ${table} +<point> +#+end_src" + (should + (equal "first one second two third three" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-3-columns () + "Associative array tests (more than 2 columns) + +No associative arrays for generic. The shell will see all values +as a single string." + (org-test-with-temp-text + "#+NAME: sample_big_table +| bread | 2 | kg | +| spaghetti | 20 | cm | +| milk | 50 | dl | + +#+begin_src sh :exports results :results output :var table=sample_big_table +echo ${table} +<point> +#+end_src" + (should + (equal "bread 2 kg spaghetti 20 cm milk 50 dl" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/bash-uses-assoc-arrays () "Bash associative arrays" -- 2.38.1 [-- Attachment #5: 0004-Refactor-ob-shell-bash-uses-assoc-arrays.patch --] [-- Type: application/octet-stream, Size: 2885 bytes --] From 2a0f5a5153546599bd673f7724250a921896c4ca Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Sat, 31 Dec 2022 12:56:51 -0500 Subject: [PATCH 4/7] Refactor ob-shell/bash-uses-assoc-arrays * testing/lisp/test-ob-shell.el (ob-shell/bash-uses-assoc-arrays): Split test cases into separate tests, `test-ob-shell/bash-uses-assoc-arrays' and `test-ob-shell/bash-uses-assoc-arrays-with-lists'. Rename prefix from "ob-shell" to "test-ob-shell". (ob-shell/bash-uses-assoc-arrays): Move comments from testing/examples/ob-shell-test.org to docstrings. (ob-shell/bash-uses-assoc-arrays): Remove dependency on testing/examples/ob-shell-test.org. --- testing/lisp/test-ob-shell.el | 53 +++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index a39c99d9f..78267880b 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -118,21 +118,44 @@ echo ${table} (equal "bread 2 kg spaghetti 20 cm milk 50 dl" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-assoc-arrays () - "Bash associative arrays" - (should - (equal "two" - (org-test-at-id - "bec1a5b0-4619-4450-a8c0-2a746b44bf8d" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block))))) - ;; Bash associative arrays as strings for the row. - (should - (equal "20 cm" - (org-test-at-id - "82320a48-3409-49d7-85c9-5de1c6d3ff87" - (org-babel-next-src-block 2) - (org-trim (org-babel-execute-src-block)))))) +(ert-deftest test-ob-shell/bash-uses-assoc-arrays () + "Bash shell: support for associative arrays + +Bash will see a table that contains the first column as the +'index' of the associative array, and the second column as the +value. " + (org-test-with-temp-text + "#+NAME: sample_mapping_table +| first | one | +| second | two | +| third | three | + +#+begin_src bash :exports :results output results :var table=sample_mapping_table +echo ${table[second]} +<point> +#+end_src " + (should + (equal "two" + (org-trim (org-babel-execute-src-block)))))) + +(ert-deftest test-ob-shell/bash-uses-assoc-arrays-with-lists () + "Bash shell: support for associative arrays with lists + +Bash will see an associative array that contains each row as a single +string. Bash cannot handle lists in associative arrays." + (org-test-with-temp-text + "#+NAME: sample_big_table +| bread | 2 | kg | +| spaghetti | 20 | cm | +| milk | 50 | dl | + +#+begin_src bash :exports results :results output :var table=sample_big_table +echo ${table[spaghetti]} +<point> +#+end_src" + (should + (equal "20 cm" + (org-trim (org-babel-execute-src-block)))))) (ert-deftest ob-shell/simple-list () "Test list variables in shell." -- 2.38.1 [-- Attachment #6: 0005-test-ob-shell.el-Refactor-test-names-and-kill-test-b.patch --] [-- Type: application/octet-stream, Size: 14542 bytes --] From 3dbc7849a366740fc507063abd048dbe04e73630 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 12:39:47 -0500 Subject: [PATCH 5/7] test-ob-shell.el: Refactor test names and kill test buffers * testing/lisp/test-ob-shell.el: (ob-shell/generic-uses-no-arrays): Rename test from `ob-shell/generic-uses-no-arrays' to `test-ob-shell/generic-uses-no-arrays'. (ob-shell/generic-uses-no-arrays): Move comments from testing/examples/ob-shell-test.org to docstring. (ob-shell/generic-uses-no-arrays): Remove dependency on testing/examples/ob-shell-test.org. (ob-shell/bash-uses-arrays): Rename `ob-shell/bash-uses-arrays' to `test-ob-shell/bash-uses-arrays'. (ob-shell/bash-uses-arrays): Move comments from testing/examples/ob-shell-test.org to docstring. (ob-shell/bash-uses-arrays): Remove dependency on testing/examples/ob-shell-test.org. (ob-shell/simple-list): Change test name from `ob-shell/simple-list' to `test-ob-shell/simple-list'. (ob-shell/remote-with-stdin-or-cmdline): Change test name from `ob-shell/remote-with-stdin-or-cmdline' to `test-ob-shell/remote-with-stdin-or-cmdline'. (ob-shell/remote-with-stdin-or-cmdline): On pass, kill buffer created by test. (ob-shell/results-table): Rename `ob-shell/results-table' to `test-ob-shell/results-table'. (ob-shell/results-list): Rename `ob-shell/results-list' to `test-ob-shell/results-list'. (ob-shell/standard-output-after-success): Rename `ob-shell/standard-output-after-success' to `test-ob-shell/standard-output-after-success'. (ob-shell/standard-output-after-failure): Rename `ob-shell/standard-output-after-failure' to `test-ob-shell/standard-output-after-failure'. (ob-shell/standard-output-after-failure): On pass, kill buffer created during test. (ob-shell/error-output-after-success): Rename `ob-shell/error-output-after-success' to `test-ob-shell/error-output-after-success'. (ob-shell/error-output-after-success): On pass, kill buffer created by test. (ob-shell/error-output-after-failure): Rename `ob-shell/error-output-after-failure' to `test-ob-shell/error-output-after-failure'. (ob-shell/error-output-after-failure): On pass, kill buffer created by test. (ob-shell/error-output-after-failure-multiple): Rename `ob-shell/error-output-after-failure-multiple' to `test-ob-shell/error-output-after-failure-multiple'. (ob-shell/error-output-after-failure-multiple): On pass, kill buffer created by test. (ob-shell/exit-code): Rename `ob-shell/exit-code' to `test-ob-shell/exit-code'. (ob-shell/exit-code): On pass, kill buffer created by test. (ob-shell/exit-code-multiple): Rename `ob-shell/exit-code-multiple' to `test-ob-shell/exit-code-multiple'. (ob-shell/exit-code-multiple): On pass, kill buffer created by test. --- testing/lisp/test-ob-shell.el | 220 ++++++++++++++++++++-------------- 1 file changed, 133 insertions(+), 87 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 78267880b..3ee81d6b8 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -18,21 +18,23 @@ ;; You should have received a copy of the GNU General Public License ;; along with this program. If not, see <https://www.gnu.org/licenses/>. -;;; Comment: -;; Template test file for Org tests +;;; Comment: -;;; Code: +;;; Requirements: (org-test-for-executable "sh") + (require 'ob-core) + (unless (featurep 'ob-shell) (signal 'missing-test-dependency "Support for Shell code blocks")) +;;; Code: (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () - "Expanded shell bodies should not start with a blank line -unless the body of the tangled block does." + "Expanded shell bodies should not start with a blank line unless +the body of the tangled block does." (should-not (string-match "^[\n\r][\t ]*[\n\r]" - (org-babel-expand-body:generic "echo 2" '()))) + (org-babel-expand-body:generic "echo 2" '()))) (should (string-match "^[\n\r][\t ]*[\n\r]" (org-babel-expand-body:generic "\n\necho 2" '())))) @@ -68,16 +70,36 @@ unless the body of the tangled block does." (if (should (equal '((1) (2)) result)) (kill-buffer session-name)))) -(ert-deftest ob-shell/generic-uses-no-arrays () - "No arrays for generic" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block) +(ert-deftest test-ob-shell/generic-uses-no-arrays () + "Test generic serialization of array into a single string." + (org-test-with-temp-text + " #+NAME: sample_array +| one | +| two | +| three | + +#+begin_src sh :exports results :results output :var array=sample_array +echo ${array} +<point> +#+end_src" (should (equal "one two three" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/bash-uses-arrays () - "Bash arrays" - (org-test-at-id "0ba56632-8dc1-405c-a083-c204bae477cf" - (org-babel-next-src-block 2) +(ert-deftest test-ob-shell/bash-uses-arrays () + "Bash sees named array as a simple indexed array. + +In this test, we check that the returned value is indeed only the +first item of the array, as opposed to the generic serialiation +that will return all elements of the array as a single string." + (org-test-with-temp-text + "#+NAME: sample_array +| one | +| two | +| three | + +#+begin_src bash :exports results :results output :var array=sample_array +echo ${array} +<point> +#+end_src" (should (equal "one" (org-trim (org-babel-execute-src-block)))))) (ert-deftest test-ob-shell/generic-uses-no-assoc-arrays-simple-map () @@ -157,22 +179,27 @@ echo ${table[spaghetti]} (equal "20 cm" (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/simple-list () - "Test list variables in shell." - ;; With bash, a list is turned into an array. +(ert-deftest test-ob-shell/simple-list () + "Test list variables." + ;; bash: a list is turned into an array (should (equal "2" - (org-test-with-temp-text - "#+BEGIN_SRC bash :results output :var l='(1 2)\necho ${l[1]}\n#+END_SRC" - (org-trim (org-babel-execute-src-block))))) - ;; On sh, it is a string containing all values. + (org-test-with-temp-text + "#+BEGIN_SRC bash :results output :var l='(1 2) + echo ${l[1]} + #+END_SRC" + (org-trim (org-babel-execute-src-block))))) + + ;; sh: a list is a string containing all values (should (equal "1 2" - (org-test-with-temp-text - "#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC" - (org-trim (org-babel-execute-src-block)))))) + (org-test-with-temp-text + "#+BEGIN_SRC sh :results output :var l='(1 2) + echo ${l} + #+END_SRC" + (org-trim (org-babel-execute-src-block)))))) -(ert-deftest ob-shell/remote-with-stdin-or-cmdline () +(ert-deftest test-ob-shell/remote-with-stdin-or-cmdline () "Test :stdin and :cmdline with a remote directory." ;; We assume `default-directory' is a local directory. (skip-unless (not (memq system-type '(ms-dos windows-nt)))) @@ -219,20 +246,27 @@ echo ${table[spaghetti]} (org-trim (org-babel-execute-src-block)))) (expected (concat "ARGS: --verbose 23 71" "\nhello tramp from " (file-local-name default-directory)))) - (should (equal result expected))))))) + (if (should (equal result expected)) + (kill-matching-buffers (format "\\*tramp/mock\\s-%s\\*" system-name) t t))))))) -(ert-deftest ob-shell/results-table () +(ert-deftest test-ob-shell/results-table () "Test :results table." (should (equal '(("I \"want\" it all")) - (org-test-with-temp-text - "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC" - (org-babel-execute-src-block))))) + (org-test-with-temp-text + "#+BEGIN_SRC sh :results table + echo 'I \"want\" it all' + #+END_SRC" + (org-babel-execute-src-block))))) -(ert-deftest ob-shell/results-list () +(ert-deftest test-ob-shell/results-list () "Test :results list." (org-test-with-temp-text - "#+BEGIN_SRC sh :results list\necho 1\necho 2\necho 3\n#+END_SRC" + "#+BEGIN_SRC sh :results list +echo 1 +echo 2 +echo 3 +#+END_SRC" (should (equal '((1) (2) (3)) (org-babel-execute-src-block))) @@ -245,86 +279,98 @@ echo ${table[spaghetti]} ;;; Standard output -(ert-deftest ob-shell/standard-output-after-success () +(ert-deftest test-ob-shell/standard-output-after-success () "Test standard output after exiting with a zero code." (should (= 1 (org-babel-execute:sh "echo 1" nil)))) -(ert-deftest ob-shell/standard-output-after-failure () +(ert-deftest test-ob-shell/standard-output-after-failure () "Test standard output after exiting with a non-zero code." - (should (= 1 - (org-babel-execute:sh - "echo 1; exit 2" nil)))) + (if + (should (= 1 + (org-babel-execute:sh + "echo 1; exit 2" nil))) + (kill-buffer "*Org-Babel Error Output*"))) ;;; Standard error -(ert-deftest ob-shell/error-output-after-success () - "Test that standard error shows in the error buffer, alongside the -exit code, after exiting with a zero code." - (should - (string= "1 +(ert-deftest test-ob-shell/error-output-after-success () + "Test that standard error shows in the error buffer, alongside +the exit code, after exiting with a zero code." + (if + (should + (string= "1 [ Babel evaluation exited with code 0 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) - -(ert-deftest ob-shell/error-output-after-failure () - "Test that standard error shows in the error buffer, alongside the -exit code, after exiting with a non-zero code." - (should - (string= "1 + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) + +(ert-deftest test-ob-shell/error-output-after-failure () + "Test that standard error shows in the error buffer, alongside +the exit code, after exiting with a non-zero code." + (if + (should + (string= "1 [ Babel evaluation exited with code 2 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2; exit 2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2; exit 2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) -(ert-deftest ob-shell/error-output-after-failure-multiple () +(ert-deftest test-ob-shell/error-output-after-failure-multiple () "Test that multiple standard error strings show in the error buffer, alongside multiple exit codes." - (should - (string= "1 + (if + (should + (string= "1 [ Babel evaluation exited with code 2 ] 3 [ Babel evaluation exited with code 4 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "echo 1 >&2; exit 2" nil) - (org-babel-execute:sh - "echo 3 >&2; exit 4" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "echo 1 >&2; exit 2" nil) + (org-babel-execute:sh + "echo 3 >&2; exit 4" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) ;;; Exit codes -(ert-deftest ob-shell/exit-code () +(ert-deftest test-ob-shell/exit-code () "Test that the exit code shows in the error buffer after exiting with a non-zero return code." - (should - (string= "[ Babel evaluation exited with code 1 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "exit 1" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) - -(ert-deftest ob-shell/exit-code-multiple () + (if + (should + (string= "[ Babel evaluation exited with code 1 ]" + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "exit 1" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) + +(ert-deftest test-ob-shell/exit-code-multiple () "Test that multiple exit codes show in the error buffer after exiting with a non-zero return code multiple times." - (should - (string= "[ Babel evaluation exited with code 1 ] + (if + (should + (string= "[ Babel evaluation exited with code 1 ] [ Babel evaluation exited with code 2 ]" - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute:sh - "exit 1" nil) - (org-babel-execute:sh - "exit 2" nil) - (with-current-buffer org-babel-error-buffer-name - (buffer-string)))))) + (progn (org-babel-eval-wipe-error-buffer) + (org-babel-execute:sh + "exit 1" nil) + (org-babel-execute:sh + "exit 2" nil) + (with-current-buffer org-babel-error-buffer-name + (buffer-string))))) + (kill-buffer "*Org-Babel Error Output*"))) (provide 'test-ob-shell) -- 2.38.1 [-- Attachment #7: 0006-ob-shell-test.org-Remove-ob-shell-test.org.patch --] [-- Type: application/octet-stream, Size: 3136 bytes --] From 9485450222cda5c3f09e02f6055aefea128ea215 Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Sat, 24 Dec 2022 11:29:06 -0500 Subject: [PATCH 6/7] ob-shell-test.org: Remove ob-shell-test.org * testing/examples/ob-shell-test.org: Delete file File was used by test-ob-shell.el to test ob-shell.el. Tests have been updated to use temporary buffers, rendering ob-shell-test.org unnecessary. --- testing/examples/ob-shell-test.org | 88 ------------------------------ 1 file changed, 88 deletions(-) delete mode 100644 testing/examples/ob-shell-test.org diff --git a/testing/examples/ob-shell-test.org b/testing/examples/ob-shell-test.org deleted file mode 100644 index 2510f4f96..000000000 --- a/testing/examples/ob-shell-test.org +++ /dev/null @@ -1,88 +0,0 @@ -#+Title: a collection of examples for ob-shell tests -#+OPTIONS: ^:nil - -* Sample data structures -#+NAME: sample_array -| one | -| two | -| three | - -#+NAME: sample_mapping_table -| first | one | -| second | two | -| third | three | - -#+NAME: sample_big_table -| bread | 2 | kg | -| spaghetti | 20 | cm | -| milk | 50 | dl | - -* Array tests - :PROPERTIES: - :ID: 0ba56632-8dc1-405c-a083-c204bae477cf - :END: -** Generic shell: no arrays -#+begin_src sh :exports results :results output :var array=sample_array -echo ${array} -#+end_src - -#+RESULTS: -: one two three - -** Bash shell: support for arrays -Bash will see a simple indexed array. In this test, we check that the -returned value is indeed only the first item of the array, as opposed to -the generic serialiation that will return all elements of the array as -a single string. -#+begin_src bash :exports results :results output :var array=sample_array -echo ${array} -#+end_src - -#+RESULTS: -: one - -* Associative array tests (simple map) - :PROPERTIES: - :ID: bec1a5b0-4619-4450-a8c0-2a746b44bf8d - :END: -** Generic shell: no special handing -The shell will see all values as a single string. -#+begin_src sh :exports results :results output :var table=sample_mapping_table -echo ${table} -#+end_src - -#+RESULTS: -: first one second two third three - -** Bash shell: support for associative arrays -Bash will see a table that contains the first column as the 'index' -of the associative array, and the second column as the value. -#+begin_src bash :exports :results output results :var table=sample_mapping_table -echo ${table[second]} -#+end_src - -#+RESULTS: -: two - -* Associative array tests (more than 2 columns) - :PROPERTIES: - :ID: 82320a48-3409-49d7-85c9-5de1c6d3ff87 - :END: -** Generic shell: no special handing -#+begin_src sh :exports results :results output :var table=sample_big_table -echo ${table} -#+end_src - -#+RESULTS: -: bread 2 kg spaghetti 20 cm milk 50 dl - -** Bash shell: support for associative arrays with lists -Bash will see an associative array that contains each row as a single -string. Bash cannot handle lists in associative arrays. -#+begin_src bash :exports results :results output :var table=sample_big_table -echo ${table[spaghetti]} -#+end_src - -#+RESULTS: -: 20 cm - -- 2.38.1 [-- Attachment #8: 0007-test-ob-shell.el-Organize-tests.patch --] [-- Type: application/octet-stream, Size: 1918 bytes --] From 9ed2821053cf7b0c2f02fe941a54e1d3a9837aeb Mon Sep 17 00:00:00 2001 From: Matt Trzcinski <matt@excalamus.com> Date: Fri, 30 Dec 2022 13:03:59 -0500 Subject: [PATCH 7/7] test-ob-shell.el: Organize tests * testing/lisp/test-ob-shell.el: - Give instructions on how to run tests - Require `org-test' explicitly - Group tests into sections - Insert `page-delimiter's between sections --- testing/lisp/test-ob-shell.el | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el index 3ee81d6b8..5318425e6 100644 --- a/testing/lisp/test-ob-shell.el +++ b/testing/lisp/test-ob-shell.el @@ -18,17 +18,22 @@ ;; You should have received a copy of the GNU General Public License ;; along with this program. If not, see <https://www.gnu.org/licenses/>. - +\f ;;; Comment: +;; See testing/README for how to run tests. + +\f ;;; Requirements: -(org-test-for-executable "sh") (require 'ob-core) (unless (featurep 'ob-shell) (signal 'missing-test-dependency "Support for Shell code blocks")) +(org-test-for-executable "sh") + +\f ;;; Code: (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies () "Expanded shell bodies should not start with a blank line unless @@ -277,6 +282,7 @@ echo 3 "- 1\n- 2\n- 3\n" (buffer-substring-no-properties (point) (point-max)))))) +\f ;;; Standard output (ert-deftest test-ob-shell/standard-output-after-success () @@ -293,6 +299,7 @@ echo 3 "echo 1; exit 2" nil))) (kill-buffer "*Org-Babel Error Output*"))) +\f ;;; Standard error (ert-deftest test-ob-shell/error-output-after-success () @@ -341,6 +348,7 @@ buffer, alongside multiple exit codes." (buffer-string))))) (kill-buffer "*Org-Babel Error Output*"))) +\f ;;; Exit codes (ert-deftest test-ob-shell/exit-code () -- 2.38.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-01 23:55 ` Matt @ 2023-01-02 9:47 ` Ihor Radchenko 2023-01-02 16:40 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-02 9:47 UTC (permalink / raw) To: Matt; +Cc: emacs-orgmode Matt <matt@excalamus.com> writes: > If this set of patches look good, I can push them to main. Just one more comment. You are using constructs like (if (should ...) (kill-buffer ...)) They will not be reliable when tests are executed interactively. If the `should' condition fails, `kill-buffer' will never be executed leaving dirty state, especially for sessions. You can instead use (unwind-protect (should ...) (kill-buffer ...)) -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-02 9:47 ` Ihor Radchenko @ 2023-01-02 16:40 ` Matt 2023-01-03 10:50 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2023-01-02 16:40 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode ---- On Mon, 02 Jan 2023 04:47:10 -0500 Ihor Radchenko wrote --- > They will not be reliable when tests are executed interactively. > If the `should' condition fails, `kill-buffer' will never be executed > leaving dirty state, especially for sessions. From my perspective, that's the point and a good thing. That "dirty state" information may be crucial to understanding why the failure happened. Can you explain why you would want a failed test to remove failure related information? ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-02 16:40 ` Matt @ 2023-01-03 10:50 ` Ihor Radchenko 2023-01-03 13:00 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-03 10:50 UTC (permalink / raw) To: Matt; +Cc: emacs-orgmode Matt <matt@excalamus.com> writes: > ---- On Mon, 02 Jan 2023 04:47:10 -0500 Ihor Radchenko wrote --- > > They will not be reliable when tests are executed interactively. > > If the `should' condition fails, `kill-buffer' will never be executed > > leaving dirty state, especially for sessions. > > From my perspective, that's the point and a good thing. That "dirty state" information may be crucial to understanding why the failure happened. Fair point. I was mostly worried about session states affecting subsequent test invocations. But I do agree that it may be better to keep them. Feel free to push upstream. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-03 10:50 ` Ihor Radchenko @ 2023-01-03 13:00 ` Matt 2023-01-05 10:31 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2023-01-03 13:00 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode ---- On Tue, 03 Jan 2023 05:50:17 -0500 Ihor Radchenko wrote --- > I was mostly worried about session states affecting subsequent test > invocations. But I do agree that it may be better to keep them. That makes sense. I tend to run tests one at a time unless I'm about to submit patches or push... > Feel free to push upstream. I'm not able to push to git://git.sv.gnu.org/emacs/org-mode.git. I've read through the following and, as far as I can tell, I've followed the directions. - https://orgmode.org/worg/org-contribute.html - https://orgmode.org/worg/org-maintenance.html - https://www.emacswiki.org/emacs/GitForEmacsDevs ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-03 13:00 ` Matt @ 2023-01-05 10:31 ` Ihor Radchenko 2023-01-05 11:21 ` Bastien Guerry 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-05 10:31 UTC (permalink / raw) To: Matt, Bastien; +Cc: emacs-orgmode Matt <matt@excalamus.com> writes: > > Feel free to push upstream. > > I'm not able to push to git://git.sv.gnu.org/emacs/org-mode.git. > > I've read through the following and, as far as I can tell, I've followed the directions. > > - https://orgmode.org/worg/org-contribute.html > - https://orgmode.org/worg/org-maintenance.html > - https://www.emacswiki.org/emacs/GitForEmacsDevs Adding Bastien to the loop. Meanwhile, can you check if you are listed as a member of "emacs" group in <https://savannah.gnu.org/my/groups.php>? -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-05 10:31 ` Ihor Radchenko @ 2023-01-05 11:21 ` Bastien Guerry 2023-01-10 2:31 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Bastien Guerry @ 2023-01-05 11:21 UTC (permalink / raw) To: Ihor Radchenko; +Cc: Matt, emacs-orgmode Ihor Radchenko <yantar92@posteo.net> writes: >> I'm not able to push to git://git.sv.gnu.org/emacs/org-mode.git. My bad: I did not warn Emacs maintainers in time. Now it is done, I will let you know when they grand you access to the Emacs project. -- Bastien ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-05 11:21 ` Bastien Guerry @ 2023-01-10 2:31 ` Matt 2023-01-11 11:53 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2023-01-10 2:31 UTC (permalink / raw) To: Bastien Guerry; +Cc: ihor radchenko, emacs-orgmode ---- On Thu, 05 Jan 2023 06:21:16 -0500 Bastien Guerry wrote --- > My bad: I did not warn Emacs maintainers in time. Now it is done, > I will let you know when they grand you access to the Emacs project. I got an email from Eli on Thursday saying I was added. I've still been getting an error when trying to push: fatal: remote error: access denied or repository not exported: /emacs/org-mode.git ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-10 2:31 ` Matt @ 2023-01-11 11:53 ` Ihor Radchenko 2023-01-11 16:18 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-11 11:53 UTC (permalink / raw) To: Matt; +Cc: Bastien Guerry, emacs-orgmode Matt <matt@excalamus.com> writes: > ---- On Thu, 05 Jan 2023 06:21:16 -0500 Bastien Guerry wrote --- > > My bad: I did not warn Emacs maintainers in time. Now it is done, > > I will let you know when they grand you access to the Emacs project. > > I got an email from Eli on Thursday saying I was added. I've still been getting an error when trying to push: > > fatal: remote error: access denied or repository not exported: /emacs/org-mode.git I have the following Git configuration: u remote.origin.url yantar92@git.savannah.gnu.org:/srv/git/emacs/org-mode.git Are you using the same? -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-11 11:53 ` Ihor Radchenko @ 2023-01-11 16:18 ` Matt 2023-01-11 17:02 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2023-01-11 16:18 UTC (permalink / raw) To: Ihor Radchenko; +Cc: bastien guerry, emacs-orgmode ---- On Wed, 11 Jan 2023 06:53:41 -0500 Ihor Radchenko wrote --- > I have the following Git configuration: > > u remote.origin.url yantar92@git.savannah.gnu.org:/srv/git/emacs/org-mode.git > > Are you using the same? I was not, thank you. I've since pushed. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-11 16:18 ` Matt @ 2023-01-11 17:02 ` Ihor Radchenko 2023-01-11 19:34 ` Matt 2023-01-12 14:43 ` Max Nikulin 0 siblings, 2 replies; 41+ messages in thread From: Ihor Radchenko @ 2023-01-11 17:02 UTC (permalink / raw) To: Matt; +Cc: bastien guerry, emacs-orgmode Matt <matt@excalamus.com> writes: > I was not, thank you. I've since pushed. This: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f319088ba5f11d4b6adf808f39f11dfa52c08e4 ? It looks like you lost all the individual commits and commit messages in the process. Could you please revert 4f319088ba5 and re-push in such a way that individual commits do appear on main? -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-11 17:02 ` Ihor Radchenko @ 2023-01-11 19:34 ` Matt 2023-01-12 8:26 ` Ihor Radchenko 2023-01-12 14:43 ` Max Nikulin 1 sibling, 1 reply; 41+ messages in thread From: Matt @ 2023-01-11 19:34 UTC (permalink / raw) To: Ihor Radchenko; +Cc: bastien guerry, emacs-orgmode ---- On Wed, 11 Jan 2023 12:02:42 -0500 Ihor Radchenko wrote --- > It looks like you lost all the individual commits and commit messages in > the process. > > Could you please revert 4f319088ba5 and re-push in such a way that > individual commits do appear on main? Goodness! Sorry! Thanks for catching that. I reverted the merge and then applied the patches. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-11 19:34 ` Matt @ 2023-01-12 8:26 ` Ihor Radchenko 0 siblings, 0 replies; 41+ messages in thread From: Ihor Radchenko @ 2023-01-12 8:26 UTC (permalink / raw) To: Matt; +Cc: bastien guerry, emacs-orgmode Matt <matt@excalamus.com> writes: > ---- On Wed, 11 Jan 2023 12:02:42 -0500 Ihor Radchenko wrote --- > > It looks like you lost all the individual commits and commit messages in > > the process. > > > > Could you please revert 4f319088ba5 and re-push in such a way that > > individual commits do appear on main? > > Goodness! Sorry! Thanks for catching that. I reverted the merge and then applied the patches. Thanks! Looks fine now. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-11 17:02 ` Ihor Radchenko 2023-01-11 19:34 ` Matt @ 2023-01-12 14:43 ` Max Nikulin 2023-01-13 9:36 ` Ihor Radchenko 1 sibling, 1 reply; 41+ messages in thread From: Max Nikulin @ 2023-01-12 14:43 UTC (permalink / raw) To: emacs-orgmode On 12/01/2023 00:02, Ihor Radchenko wrote: >> I was not, thank you. I've since pushed. > > This: > https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f319088ba5f11d4b6adf808f39f11dfa52c08e4 > ? > > It looks like you lost all the individual commits and commit messages in > the process. Ihor, it is usual merge commit of a branch with multiple commits. Cgit shows combined changes, but commits was not squashed. The branch started at dff9565c2c8bb7fe100c5278136d3de22ce41051 Bastien 2022-12-30 09:17:38 +0100 It would be better to do git fetch git rebase origin/main before merging the branch to make git history more linear. To ensure fast forward merge it is possible to add --ff-only option to git merge. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-12 14:43 ` Max Nikulin @ 2023-01-13 9:36 ` Ihor Radchenko 2023-01-13 15:18 ` Matt 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-13 9:36 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode Max Nikulin <manikulin@gmail.com> writes: >> It looks like you lost all the individual commits and commit messages in >> the process. > > Ihor, it is usual merge commit of a branch with multiple commits. Cgit > shows combined changes, but commits was not squashed. The branch started at > dff9565c2c8bb7fe100c5278136d3de22ce41051 Bastien 2022-12-30 09:17:38 +0100 Cgit displays our bugfix merges with all the required commits. So, what happened what not ideal either way. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-13 9:36 ` Ihor Radchenko @ 2023-01-13 15:18 ` Matt 2023-01-13 15:23 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2023-01-13 15:18 UTC (permalink / raw) To: Ihor Radchenko; +Cc: max nikulin, emacs-orgmode ---- On Fri, 13 Jan 2023 04:36:40 -0500 Ihor Radchenko wrote --- > Cgit displays our bugfix merges with all the required commits. > So, what happened what not ideal either way. Would you like me to correct how I've incorporated my changes? ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-13 15:18 ` Matt @ 2023-01-13 15:23 ` Ihor Radchenko 2023-01-14 7:41 ` Max Nikulin 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-13 15:23 UTC (permalink / raw) To: Matt; +Cc: max nikulin, emacs-orgmode Matt <matt@excalamus.com> writes: > ---- On Fri, 13 Jan 2023 04:36:40 -0500 Ihor Radchenko wrote --- > > Cgit displays our bugfix merges with all the required commits. > > So, what happened what not ideal either way. > > Would you like me to correct how I've incorporated my changes? No. I was referring to the initial situation with a single commit being displayed. I am not sure what Max was trying to point out. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-13 15:23 ` Ihor Radchenko @ 2023-01-14 7:41 ` Max Nikulin 2023-01-14 10:35 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Max Nikulin @ 2023-01-14 7:41 UTC (permalink / raw) To: emacs-orgmode On 13/01/2023 22:23, Ihor Radchenko wrote: > Matt writes: > >> Would you like me to correct how I've incorporated my changes? > > No. I was referring to the initial situation with a single commit being > displayed. > > I am not sure what Max was trying to point out. Look at the commit message for https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f319088ba5f11d4b6adf808f39f11dfa52c08e4 > Merge branch 'ob-shell-cleanup-tests' It suggests that this is a merge commit for a local branch. Notice that the commit has 2 parents: > committer Matthew Trzcinski 2023-01-11 11:16:53 -0500 > parent 07c63df9c7f40b17acb5c517baa0d321098f31da (diff) > parent 9ed2821053cf7b0c2f02fe941a54e1d3a9837aeb (diff) Second one: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=9ed2821053cf7b0c2f02fe941a54e1d3a9837aeb author Matt Trzcinski 2022-12-30 13:03:59 -0500 committer Matt Trzcinski 2022-12-31 13:56:27 -0500 test-ob-shell.el: Organize tests And changes made by this commit are included into diff shown for the merge commit 4f319088ba by cgit. E.g. gitk for local repository does not show any changes for the merge commit. So Matt did not squashed commits before committing to the main branch and detailed commit messages are preserved. That is why I do not consider cgit render as a strong enough reason for reverting. However I would prefer linear commit history when possible, so I suggest to do the following in similar cases (not verified, may have typos) git fetch # get latest commits git checkout ob-shell-cleanup-tests git rebase origin/main git checkout main git pull git merge --ff-only ob-shell-cleanup-tests (Omit --ff-only if it is real merge because the fix is committed to the bugfix branch.) It should help to avoid confusion and to make git archeology easier. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2023-01-14 7:41 ` Max Nikulin @ 2023-01-14 10:35 ` Ihor Radchenko 2023-01-14 15:09 ` cgit and merge commit Max Nikulin 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-14 10:35 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode Max Nikulin <manikulin@gmail.com> writes: > And changes made by this commit are included into diff shown for the > merge commit 4f319088ba by cgit. E.g. gitk for local repository does not > show any changes for the merge commit. > > So Matt did not squashed commits before committing to the main branch > and detailed commit messages are preserved. That is why I do not > consider cgit render as a strong enough reason for reverting. Should we then report a bug to cgit mailing list? > However I would prefer linear commit history when possible, so I suggest > to do the following in similar cases (not verified, may have typos) > > git fetch # get latest commits > git checkout ob-shell-cleanup-tests > git rebase origin/main > git checkout main > git pull > git merge --ff-only ob-shell-cleanup-tests > > (Omit --ff-only if it is real merge because the fix is committed to the > bugfix branch.) > > It should help to avoid confusion and to make git archeology easier. Sure. Using rebase for merging and pulling is generally a good idea. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* cgit and merge commit 2023-01-14 10:35 ` Ihor Radchenko @ 2023-01-14 15:09 ` Max Nikulin 2023-01-24 20:16 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Max Nikulin @ 2023-01-14 15:09 UTC (permalink / raw) To: emacs-orgmode On 14/01/2023 17:35, Ihor Radchenko wrote: > Max Nikulin writes: >> >> So Matt did not squashed commits before committing to the main branch >> and detailed commit messages are preserved. That is why I do not >> consider cgit render as a strong enough reason for reverting. > > Should we then report a bug to cgit mailing list? I do not have particular opinion. I rarely use web UI for git and mostly for blame. I prefer to do it with local clone. I have never read cgit docs. Perhaps it may be more apparent that it is a merge commit. Anyway at first I would ask the savannah team. E.g. vanilla debbugs instance is significantly more convenient than the GNU ones and the changes in the latter are intentional. Mhonarc (list archive) at lists.debian.org has better configuration than at lists.gnu.org as well. So there might be specific of particular instance. After all, it may be convenient to review cumulative changes. When I worked with gerrit (a web application for code review) sometimes it was inconvenient that it was impossible to see diff for whole commit series. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: cgit and merge commit 2023-01-14 15:09 ` cgit and merge commit Max Nikulin @ 2023-01-24 20:16 ` Ihor Radchenko 0 siblings, 0 replies; 41+ messages in thread From: Ihor Radchenko @ 2023-01-24 20:16 UTC (permalink / raw) To: Max Nikulin; +Cc: emacs-orgmode Max Nikulin <manikulin@gmail.com> writes: >> Should we then report a bug to cgit mailing list? > > I do not have particular opinion. I rarely use web UI for git and mostly > for blame. I prefer to do it with local clone. I have never read cgit > docs. Perhaps it may be more apparent that it is a merge commit. > > Anyway at first I would ask the savannah team. E.g. vanilla debbugs > instance is significantly more convenient than the GNU ones and the > changes in the latter are intentional. Mhonarc (list archive) at > lists.debian.org has better configuration than at lists.gnu.org as well. > So there might be specific of particular instance. I asked savannah people, and they pointed out that the commits where, in fact, listed in the logs. Just at their original creating dates - much earlier than the merge commit date and further down into the logs. So, it was mostly my oversight from the very beginning - nothing was lost during the merge process. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: ob-shell intentions and paperwork (was Bash results broken?) 2022-12-30 5:34 ` Matt 2022-12-30 8:06 ` Bastien Guerry 2022-12-30 18:46 ` Matt @ 2022-12-31 12:56 ` Ihor Radchenko 2023-01-02 4:40 ` Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) Matt 2 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2022-12-31 12:56 UTC (permalink / raw) To: Matt; +Cc: bastien, emacs-orgmode Matt <matt@excalamus.com> writes: > > > +;; TODO refactor into macro. Currently violates (elisp) Coding > > > +;; Conventions and is hard to debug. > > > (defun org-babel-shell-initialize () > > > "Define execution functions associated to shell names. > > > > Could you please elaborate? Which particular convention does it violate? > > What is hard to debug? > > (elisp) Coding Conventions says, > > "• Constructs that define a function or variable should be macros, not > functions, and their names should start with ‘define-’. The macro > should receive the name to be defined as the first argument. That > will help various tools find the definition automatically. Avoid > constructing the names in the macro itself, since that would > confuse these tools." > > The `org-babel-shell-initialize' function defines *all* the `org-babel-execute:XXX' functions given by `org-babel-shell-names' (sh, bash, zsh, etc.). I agree that `org-babel-shell-initialize' could use a better name. As for being a macro, there will be not much gain - the convention is mostly designed for things like `cl-defun' aimed to be used in the code. `org-babel-shell-initialize' is only used by `org-babel-shell-names'. I do not have objections if it were a macro though. (But I do not see how it would help debugging). > Because `org-babel-shell-initialize' is a function factory, you can't easily examine or modify their definitions. `C-h f org-babel-execute:sh' jumps to the top of lisp/ob-shell.el. Changing the definition requires reevaluating the definition for all the execute functions (or first changing `org-babel-shell-names'). This is indeed a downside. Any better ideas? ob-core dictates that we must have org-babel-execute:lang functions to make things work. > This was a problem for me when I wanted to make the session name string for `test-ob-shell/session' the test name (mentioned above). In the test, when I replaced the session name string with a variable containing the string, `org-babel-execute:sh' failed with a type error. I couldn't get the variable to evaluate (with backquote and comma or otherwise). Without an explicit function definition or a macro to expand, I found it hard to debug/experiment with (and so left the test name as a hard coded string). Could you please explain a bit more about the problem? I do not see how macro would help in this situation. > I probably don't need it and am happy to remove it. An older version of the function was more complex and made sense as a separate function (or so I thought). My aim was to make the test strings easy to read so that it's clearer what's being tested (i.e. not write multi-line strings on a single line). I could use concat and add "\n" to the end of each line. Or, simply write out the string-join. Maybe there's another way to write multi-line strings that I'm not aware of in Elisp, maybe something like Python's triple-quote? We write multi-line docstrings all the time without extra macros. I recommend using paredit or similar packages to auto-escape things that need to be escaped. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) 2022-12-31 12:56 ` ob-shell intentions and paperwork (was Bash results broken?) Ihor Radchenko @ 2023-01-02 4:40 ` Matt 2023-01-03 9:29 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Matt @ 2023-01-02 4:40 UTC (permalink / raw) To: Ihor Radchenko; +Cc: emacs-orgmode ---- On Sat, 31 Dec 2022 07:56:10 -0500 Ihor Radchenko wrote --- > As for being a macro, there will be not much gain - the convention is > mostly designed for things like `cl-defun' aimed to be used in the code. > `org-babel-shell-initialize' is only used by `org-babel-shell-names'. I'm not sure what you mean by "to be used in code". Do you mean called within the global scope? > I do not have objections if it were a macro though. (But I do not see > how it would help debugging). > > > Because `org-babel-shell-initialize' is a function factory, you can't easily examine or modify their definitions. `C-h f org-babel-execute:sh' jumps to the top of lisp/ob-shell.el. Changing the definition requires reevaluating the definition for all the execute functions (or first changing `org-babel-shell-names'). > > This is indeed a downside. Any better ideas? > ob-core dictates that we must have org-babel-execute:lang functions to > make things work. My apologies, I feel there are too many separate issues I've brought up. Since I've already brought them up, let me try to be more clear about them. I observe four issues with the current form of `org-babel-shell-initialize': 1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug 2. Source navigation gets confused when looking up help for a generated function. For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el 3. It does not adhere to the coding convention 4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator, currently `org-babel-shell-initialize' Here's my perspective on each point. 1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug The benefit of using a macro is clarity of the defined functions. Here's the core `org-babel-shell-initialize' functionality as a macro. Note that it doesn't loop through `org-babel-shell-names'. (defmacro define-babel-shell-1 (shell-name) (declare (indent nil) (debug t)) (let ((function-name (intern (concat "my-org-babel-execute:" shell-name)))) `(progn (defun ,function-name (body params) ,(format "Execute a block of %s commands with Babel." shell-name) (let ((shell-file-name ,shell-name) (org-babel-prompt-command (or (alist-get ,shell-name org-babel-shell-set-prompt-commands) (alist-get t org-babel-shell-set-prompt-commands)))) (org-babel-execute:shell body params))) (defalias ',(intern (concat "org-babel-variable-assignments:" shell-name)) 'org-babel-variable-assignments:shell ,(format "Return list of %s statements assigning to the block's variables." shell-name)) (defvar ,(intern (concat "org-babel-default-header-args:" shell-name)) nil)))) You can expand to see the definitions: (pp-macroexpand-expression '(define-babel-shell-1 "csh")) Is there a way to see the definition of`org-babel-execute:csh' using the current `org-babel-shell-initialize', that is, when generated by a function? Looking at the expansion, I see what appears to be an error: (alist-get "csh" org-babel-shell-set-prompt-commands) `org-babel-shell-set-prompt-commands' is an alist keyed by string shell names and whose values are shell commands to set the prompt. `alist-get' uses `eq' by default which always returns nil for string comparisons. That is, (alist-get "csh" org-babel-shell-set-prompt-commands) returns nil, not because the corresponding alist value is nil. Rather, because the (eq "csh" "csh") comparison evaluates as nil. The TESTFN probably should be `equal' or `string=': (alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal) Then, it will return the prompt associated with "csh". All this is visible from the function version of `org-babel-shell-initialize', of course. However, it requires mentally tracking that ,name resolves to a string. Using a macro along with `pp-macroexpand-expression' makes the defined function explicit. The forms generated by the macro expansion are readily available to eval whereas the function version makes the definitions inaccessible (AFAICT). 2. Source navigation gets confused when looking up help for a generated function. For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el Generating the execute functions with a macro also has this problem. I'm not sure there's a way around it other than defining each function with `defun'. Doing that would be a bad idea because of the massive code duplication/maintenance it would create. 3. It does not adhere to the coding convention. I'll requote the convention here for convenience. > (elisp) Coding Conventions says, > > "• Constructs that define a function or variable should be macros, not > functions, and their names should start with ‘define-’. The macro > should receive the name to be defined as the first argument. That > will help various tools find the definition automatically. Avoid > constructing the names in the macro itself, since that would > confuse these tools." It's not clear to me why the convention exists, beyond consistency (a valid reason). I suspected it was to make the code clearer (points 1) and to "help various tools find the definition automatically" (point 2). I'm biased by my experience into thinking a macro actually addresses point 1. I could be wrong about it, though. It could just have been luck and personal preference, and I may be overlooking some hidden complexity, a common problem with macros. Is there anything you see that I might be overlooking? AFAICT, a macro doesn't help with finding the definition of the generated function. Any idea what tools it's talking about? Also, the way I defined `define-babel-shell-1' doesn't fit the convention. Something like this would: (defmacro define-babel-execute-shell-2 (name) "Define functions and variables needed by Org Babel to execute a shell. NAME is a symbol of the form `org-babel-execute:my-shell'." (declare (indent nil) (debug t)) (let ((shell-program (cadr (split-string (symbol-name name) ":")))) `(progn (defun ,name (body params) ,(format "Execute a block of %s commands with Babel." shell-program) (let ((shell-file-name ,shell-program) (org-babel-prompt-command (or (alist-get ,shell-program org-babel-shell-set-prompt-commands) (alist-get t org-babel-shell-set-prompt-commands)))) (org-babel-execute:shell body params))) (defalias ',(intern (concat "org-babel-variable-assignments:" shell-program)) 'org-babel-variable-assignments:shell ,(format "Return list of %s statements assigning to the block's variables." shell-program)) (defvar ,(intern (concat "org-babel-default-header-args:" shell-program)) nil)))) AFAICT, adhering strictly to the convention would make things needlessly complicated. The execute function's symbols would need to be interned beforehand, creating an extra step between the `org-babel-shell-names' and the execute functions, only for them to be converted and parsed out in the macro: (intern "org-babel-execute:test") (symbolp 'org-babel-execute:test) (pp-macroexpand-expression '(define-babel-execute-shell-2 org-babel-execute:test)) 4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator (`org-babel-shell-initialize' or some variant of `define-babel-shell-1' ) A macro would not solve the need to re-evaluate the function generation when a change is made to `org-babel-shell-names'. Either way, we need to loop/map over the list of shells. I'm curious your thoughts about removing the `:set' function from `org-babel-shell-names' and using `add-variable-watcher' instead. In my explorations, the watcher gets called when using customize, as well as when a new shell is added to `org-babel-shell-names' using `add-to-list'. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) 2023-01-02 4:40 ` Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) Matt @ 2023-01-03 9:29 ` Ihor Radchenko 2023-01-05 8:32 ` Ihor Radchenko 0 siblings, 1 reply; 41+ messages in thread From: Ihor Radchenko @ 2023-01-03 9:29 UTC (permalink / raw) To: Matt; +Cc: emacs-orgmode Matt <matt@excalamus.com> writes: > ---- On Sat, 31 Dec 2022 07:56:10 -0500 Ihor Radchenko wrote --- > > As for being a macro, there will be not much gain - the convention is > > mostly designed for things like `cl-defun' aimed to be used in the code. > > `org-babel-shell-initialize' is only used by `org-babel-shell-names'. > > I'm not sure what you mean by "to be used in code". Do you mean called within the global scope? There are certain conventions about indentation of macros with "defun" in them. They are automatically applied in emacs-lisp-mode. Also, some heuristics for imenu looks for "defun" top-level forms, AFAIR. In all these scenarios, it is assumed that "defun" macros are used in the source code to define functions during compile time. Not on runtime. > 1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug > > The benefit of using a macro is clarity of the defined functions. Here's the core `org-babel-shell-initialize' functionality as a macro. Note that it doesn't loop through `org-babel-shell-names'. > ... > You can expand to see the definitions: > > (pp-macroexpand-expression '(define-babel-shell-1 "csh")) > > Is there a way to see the definition of`org-babel-execute:csh' using the current `org-babel-shell-initialize', that is, when generated by a function? https://github.com/Wilfred/helpful displays the function code in such scenario. Probably, I need to raise this problem on emacs-devel. > Looking at the expansion, I see what appears to be an error: > > (alist-get "csh" org-babel-shell-set-prompt-commands) > > `org-babel-shell-set-prompt-commands' is an alist keyed by string shell names and whose values are shell commands to set the prompt. `alist-get' uses `eq' by default which always returns nil for string comparisons. That is, (alist-get "csh" org-babel-shell-set-prompt-commands) returns nil, not because the corresponding alist value is nil. Rather, because the (eq "csh" "csh") comparison evaluates as nil. The TESTFN probably should be `equal' or `string=': > > (alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal) > > Then, it will return the prompt associated with "csh". Good point. Would you mind making a patch? > 2. Source navigation gets confused when looking up help for a generated function. For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el This is indeed to be expected. > Generating the execute functions with a macro also has this problem. I'm not sure there's a way around it other than defining each function with `defun'. Doing that would be a bad idea because of the massive code duplication/maintenance it would create. Yup. > 3. It does not adhere to the coding convention. > > I'll requote the convention here for convenience. > >> (elisp) Coding Conventions says, >> >> "• Constructs that define a function or variable should be macros, not >> functions, and their names should start with ‘define-’. The macro >> should receive the name to be defined as the first argument. That >> will help various tools find the definition automatically. Avoid >> constructing the names in the macro itself, since that would >> confuse these tools." > > It's not clear to me why the convention exists, beyond consistency (a valid reason). I suspected it was to make the code clearer (points 1) and to "help various tools find the definition automatically" (point 2). > > I'm biased by my experience into thinking a macro actually addresses point 1. I could be wrong about it, though. It could just have been luck and personal preference, and I may be overlooking some hidden complexity, a common problem with macros. Is there anything you see that I might be overlooking? Nothing substantial, AFAIK. > AFAICT, a macro doesn't help with finding the definition of the generated function. Any idea what tools it's talking about? See above. > Also, the way I defined `define-babel-shell-1' doesn't fit the convention. Something like this would: > > (defmacro define-babel-execute-shell-2 (name) > "Define functions and variables needed by Org Babel to execute a shell. > > NAME is a symbol of the form `org-babel-execute:my-shell'." > (declare (indent nil) (debug t)) > (let ((shell-program (cadr (split-string (symbol-name name) ":")))) Symbol argument will create awkward back-and-forth conversion between string and a symbol here. > 4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator (`org-babel-shell-initialize' or some variant of `define-babel-shell-1' ) > > A macro would not solve the need to re-evaluate the function generation when a change is made to `org-babel-shell-names'. Either way, we need to loop/map over the list of shells. > > I'm curious your thoughts about removing the `:set' function from `org-babel-shell-names' and using `add-variable-watcher' instead. In my explorations, the watcher gets called when using customize, as well as when a new shell is added to `org-babel-shell-names' using `add-to-list'. I have never seen using variable watcher for this purpose. I suggest asking on emacs-devel first to hear what they think of it. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) 2023-01-03 9:29 ` Ihor Radchenko @ 2023-01-05 8:32 ` Ihor Radchenko 0 siblings, 0 replies; 41+ messages in thread From: Ihor Radchenko @ 2023-01-05 8:32 UTC (permalink / raw) To: Matt; +Cc: emacs-orgmode Ihor Radchenko <yantar92@posteo.net> writes: >> Is there a way to see the definition of`org-babel-execute:csh' using the current `org-babel-shell-initialize', that is, when generated by a function? > > https://github.com/Wilfred/helpful displays the function code in such > scenario. Probably, I need to raise this problem on emacs-devel. https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60568 >> (alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal) >> >> Then, it will return the prompt associated with "csh". > > Good point. Would you mind making a patch? Fixed on bugfix. https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=93339de71 >> >> I'm curious your thoughts about removing the `:set' function from `org-babel-shell-names' and using `add-variable-watcher' instead. In my explorations, the watcher gets called when using customize, as well as when a new shell is added to `org-babel-shell-names' using `add-to-list'. > > I have never seen using variable watcher for this purpose. > I suggest asking on emacs-devel first to hear what they think of it. https://yhetil.org/emacs-devel/87a62xiewa.fsf@localhost/T/#u -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2023-01-24 20:17 UTC | newest] Thread overview: 41+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-16 16:22 Bash results broken? Rudolf Adamkovič 2022-12-16 17:41 ` Ihor Radchenko 2022-12-18 11:19 ` Ihor Radchenko 2022-12-20 0:44 ` Rudolf Adamkovič 2022-12-20 3:40 ` Matt 2022-12-25 11:23 ` Ihor Radchenko 2022-12-26 2:25 ` Matt 2022-12-26 9:26 ` Ihor Radchenko 2022-12-21 6:17 ` ob-shell intentions and paperwork (was Bash results broken?) Matt 2022-12-27 20:48 ` Matt 2022-12-29 11:08 ` Ihor Radchenko 2022-12-29 14:20 ` Bastien Guerry 2022-12-30 5:34 ` Matt 2022-12-30 8:06 ` Bastien Guerry 2022-12-30 18:46 ` Matt 2022-12-31 14:31 ` Ihor Radchenko 2023-01-01 23:55 ` Matt 2023-01-02 9:47 ` Ihor Radchenko 2023-01-02 16:40 ` Matt 2023-01-03 10:50 ` Ihor Radchenko 2023-01-03 13:00 ` Matt 2023-01-05 10:31 ` Ihor Radchenko 2023-01-05 11:21 ` Bastien Guerry 2023-01-10 2:31 ` Matt 2023-01-11 11:53 ` Ihor Radchenko 2023-01-11 16:18 ` Matt 2023-01-11 17:02 ` Ihor Radchenko 2023-01-11 19:34 ` Matt 2023-01-12 8:26 ` Ihor Radchenko 2023-01-12 14:43 ` Max Nikulin 2023-01-13 9:36 ` Ihor Radchenko 2023-01-13 15:18 ` Matt 2023-01-13 15:23 ` Ihor Radchenko 2023-01-14 7:41 ` Max Nikulin 2023-01-14 10:35 ` Ihor Radchenko 2023-01-14 15:09 ` cgit and merge commit Max Nikulin 2023-01-24 20:16 ` Ihor Radchenko 2022-12-31 12:56 ` ob-shell intentions and paperwork (was Bash results broken?) Ihor Radchenko 2023-01-02 4:40 ` Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) Matt 2023-01-03 9:29 ` Ihor Radchenko 2023-01-05 8:32 ` Ihor Radchenko
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs/org-mode.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).