ob-shell intentions and paperwork (was Bash results broken?)

[Matt <matt@excalamus.com>]

[-- 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