emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [PATCH] rfc: using ert-deftest with side-effects
@ 2022-11-07 17:03 Leo Butler
  2022-11-08  7:40 ` Ihor Radchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Leo Butler @ 2022-11-07 17:03 UTC (permalink / raw)
  To: Org Mode Mailing List

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

Hello,

I am patching a bug in ob-octave.el (see attachment) involving the
creation of graphics files. The bug itself is easy to fix: a single line
in ob-octave.el ensures the special variable `ans' is bound, to prevent
Octave from exiting with a non-zero exit code.

However, I would like feedback/suggestions on writing such a
test. Issues include:

1. how to clean up the side-effects, including changes in the test
   buffer, filesystem and potentially creating an error buffer;
2. the general absence of similar tests (except in test-ob.el,
   test-ob/result-graphics-link-type-header-argument).

To address 1., I have wrapped the tests in an `unwind-protect' form to
ensure clean-up code gets run. The ERT manual does not suggest much
beyond this. At the moment, when the test is run, clean-up is being done
whether the test fails or passes.

I am unsure about 2. Is the absence of such tests because there is a
policy against them, or ...

Leo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-On-ltb-ob-max-ob-octave.patch --]
[-- Type: text/x-diff; name="0001-On-ltb-ob-max-ob-octave.patch", Size: 2198 bytes --]

diff --git a/lisp/ob-octave.el b/lisp/ob-octave.el
index 55926b789..f85b79fa2 100644
--- a/lisp/ob-octave.el
+++ b/lisp/ob-octave.el
@@ -91,7 +91,7 @@ end")
 				 (list
 				  "set (0, \"defaultfigurevisible\", \"off\");"
 				  full-body
-				  (format "print -dpng %s" gfx-file))
+				  (format "print -dpng %s\nans=%S" gfx-file gfx-file))
 				 "\n")
 		    full-body)
 		  result-type matlabp)))
diff --git a/testing/examples/ob-octave-test.org b/testing/examples/ob-octave-test.org
index 9839d637e..0fc84bc26 100644
--- a/testing/examples/ob-octave-test.org
+++ b/testing/examples/ob-octave-test.org
@@ -46,10 +46,16 @@ ans = s
 
 
 * Graphical tests
-#+begin_src octave :results graphics :file chart.png
+  :PROPERTIES:
+  :ID:       b8b1d6a0-b7f6-49bd-8cb0-0c74f737332f
+  :END:
+
+Graphics file
+#+begin_src octave :results file graphics :file sombrero.png
 sombrero;
 #+end_src
 
+Graphics session
 #+begin_src octave :session
 sombrero;
 #+end_src
diff --git a/testing/lisp/test-ob-octave.el b/testing/lisp/test-ob-octave.el
index 78ce10214..bdc9befef 100644
--- a/testing/lisp/test-ob-octave.el
+++ b/testing/lisp/test-ob-octave.el
@@ -64,4 +64,22 @@
     (org-babel-next-src-block 5)
     (should (equal nil (org-babel-execute-src-block)))))
 
+(ert-deftest ob-octave/graphics-file ()
+  "Graphics file. Test that link is correctly inserted and graphics file is created (and not empty). Clean-up side-effects."
+  (org-test-at-id "b8b1d6a0-b7f6-49bd-8cb0-0c74f737332f"
+    (org-babel-next-src-block)
+    (org-babel-execute-src-block)
+    (unwind-protect
+        (prog1
+            (and (should (search-forward "[[file:sombrero.png]]" nil nil))
+                 (should (file-readable-p "sombrero.png"))
+                 (should (let ((size (nth 7 (file-attributes "sombrero.png"))))
+                           (> size 0)))
+                 (should (not (get-buffer "*Org-Babel Error Output*")))))
+      ;; clean-up
+      (delete-file "sombrero.png")
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*"))
+      (revert-buffer t t))))
+
 (provide 'test-ob-octave)

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] rfc: using ert-deftest with side-effects
  2022-11-07 17:03 [PATCH] rfc: using ert-deftest with side-effects Leo Butler
@ 2022-11-08  7:40 ` Ihor Radchenko
  2022-11-08 19:55   ` [PATCH] lisp/ob-octave.el, was " Leo Butler
  0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2022-11-08  7:40 UTC (permalink / raw)
  To: Leo Butler; +Cc: Org Mode Mailing List

Leo Butler <Leo.Butler@umanitoba.ca> writes:

> However, I would like feedback/suggestions on writing such a
> test. Issues include:
>
> 1. how to clean up the side-effects, including changes in the test
>    buffer, filesystem and potentially creating an error buffer;

As you did, we generally use unwind-protect. Also, we prefer putting
temporary files into temporary directory.

You can grep for `make-temp-file' and `delete-file' in tests. There are
plenty of examples.

> 2. the general absence of similar tests (except in test-ob.el,
>    test-ob/result-graphics-link-type-header-argument).
> ...
> I am unsure about 2. Is the absence of such tests because there is a
> policy against them, or ...

We have no such policy. In fact, many tests are making
temporary files.

-- 
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] 7+ messages in thread

* [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects
  2022-11-08  7:40 ` Ihor Radchenko
@ 2022-11-08 19:55   ` Leo Butler
  2022-11-09  5:14     ` Ihor Radchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Leo Butler @ 2022-11-08 19:55 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Org Mode Mailing List

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

On Tue, Nov 08 2022, Ihor Radchenko <yantar92@posteo.net> wrote:

> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>
>> However, I would like feedback/suggestions on writing such a
>> test. Issues include:
>>
>> 1. how to clean up the side-effects, including changes in the test
>>    buffer, filesystem and potentially creating an error buffer;
>
> As you did, we generally use unwind-protect. Also, we prefer putting
> temporary files into temporary directory.
>
> You can grep for `make-temp-file' and `delete-file' in tests. There are
> plenty of examples.
>
>> 2. the general absence of similar tests (except in test-ob.el,
>>    test-ob/result-graphics-link-type-header-argument).
>> ...
>> I am unsure about 2. Is the absence of such tests because there is a
>> policy against them, or ...
>
> We have no such policy. In fact, many tests are making
> temporary files.

Ihor,
Thanks for your feeback and the pointer. I have revised the tests and
attach the revised patch.

Best regards,
Leo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-prevent-error-in-Octave-process-add-tests-update-tes.patch --]
[-- Type: text/x-diff; name="0001-prevent-error-in-Octave-process-add-tests-update-tes.patch", Size: 4945 bytes --]

From 84587bdbd705c2769d0f02398f1e38fe26ac0a98 Mon Sep 17 00:00:00 2001
From: Leo Butler <leo.butler@umanitoba.ca>
Date: Tue, 8 Nov 2022 13:31:47 -0600
Subject: [PATCH] prevent error in Octave process, add tests, update test docs

* lisp/ob-octave.el (org-babel-execute:octave):

  Ensure that the special Octave variable `ans' is bound when GFX-FILE
  is non-nil.  The glue code in ORG-BABEL-OCTAVE-WRAPPER-METHOD causes
  Octave to exit with a non-zero exit code when `ans' is not bound.

* testing/examples/ob-octave-test.org:

  Update the Graphical tests section:
  -put in the correct headers;
  -add a remark about where to find the test.

* testing/lisp/test-ob-octave.el:

  Add the tests ob-octave/graphics-file and
  ob-octave/graphics-file-session. The first test verifies that the
  bug identified above is fixed; it also verifies that graphics file
  creation works correctly for scripting. The second test verifies
  graphics file creation works correctly for sessions.
---
 lisp/ob-octave.el                   |  2 +-
 testing/examples/ob-octave-test.org |  7 +++--
 testing/lisp/test-ob-octave.el      | 47 +++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-octave.el b/lisp/ob-octave.el
index 55926b789..f85b79fa2 100644
--- a/lisp/ob-octave.el
+++ b/lisp/ob-octave.el
@@ -91,7 +91,7 @@ end")
 				 (list
 				  "set (0, \"defaultfigurevisible\", \"off\");"
 				  full-body
-				  (format "print -dpng %s" gfx-file))
+				  (format "print -dpng %s\nans=%S" gfx-file gfx-file))
 				 "\n")
 		    full-body)
 		  result-type matlabp)))
diff --git a/testing/examples/ob-octave-test.org b/testing/examples/ob-octave-test.org
index 9839d637e..c0f04b7b9 100644
--- a/testing/examples/ob-octave-test.org
+++ b/testing/examples/ob-octave-test.org
@@ -46,10 +46,13 @@ ans = s
 
 
 * Graphical tests
-#+begin_src octave :results graphics :file chart.png
+
+Graphics file. This test is performed by =ob-octave/graphics-file= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :results file graphics :file sombrero.png
 sombrero;
 #+end_src
 
-#+begin_src octave :session
+Graphics file in a session. This test is performed by =ob-octave/graphics-file-session= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :session :results graphics file :file sombrero.png
 sombrero;
 #+end_src
diff --git a/testing/lisp/test-ob-octave.el b/testing/lisp/test-ob-octave.el
index 78ce10214..dc3782fd0 100644
--- a/testing/lisp/test-ob-octave.el
+++ b/testing/lisp/test-ob-octave.el
@@ -64,4 +64,51 @@
     (org-babel-next-src-block 5)
     (should (equal nil (org-babel-execute-src-block)))))
 
+(ert-deftest ob-octave/graphics-file ()
+  "Graphics file. Test that link is correctly inserted and graphics file is created (and not empty). Clean-up side-effects."
+  ;; In case a prior test left the Error Output buffer hanging around.
+  (when (get-buffer "*Org-Babel Error Output*")
+    (kill-buffer "*Org-Babel Error Output*"))
+  (let ((file (make-temp-file "test-ob-octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :results file graphics :file %s
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (let ((size (nth 7 (file-attributes file))))
+                    (> size 0)))
+          (should (not (get-buffer "*Org-Babel Error Output*"))))
+      ;; clean-up
+      (delete-file file)
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+(ert-deftest ob-octave/graphics-file-session ()
+  "Graphics file in a session. Test that session is started in *Inferior Octave* buffer, link is correctly inserted and graphics file is created (and not empty). Clean-up side-effects."
+  (let ((file (make-temp-file "test-ob-octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :session :results file graphics :file %s
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (get-buffer "*Inferior Octave*"))
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (let ((size (nth 7 (file-attributes file))))
+                    (> size 0)))
+          (should (not (get-buffer "*Org-Babel Error Output*"))))
+      ;; clean-up
+      (delete-file file)
+      (let (kill-buffer-query-functions kill-buffer-hook)
+        (kill-buffer "*Inferior Octave*"))
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+
 (provide 'test-ob-octave)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects
  2022-11-08 19:55   ` [PATCH] lisp/ob-octave.el, was " Leo Butler
@ 2022-11-09  5:14     ` Ihor Radchenko
  2022-11-09 20:33       ` Leo Butler
  0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2022-11-09  5:14 UTC (permalink / raw)
  To: Leo Butler; +Cc: Org Mode Mailing List

Leo Butler <Leo.Butler@umanitoba.ca> writes:

> Ihor,
> Thanks for your feeback and the pointer. I have revised the tests and
> attach the revised patch.

Thanks!

Note that your patch is over 15LOC, which exceeds legally allowed
contribution size for people without copyright assignment.

Would you be interested to sign the copyright assignment form and send
it to FSF? See https://orgmode.org/worg/org-contribute.html#copyright
for details. The process usually takes a few days on FSF side (they are
obliged to reply within 5 working days).

Below are some comments.

> * testing/lisp/test-ob-octave.el:
>
>   Add the tests ob-octave/graphics-file and
>   ob-octave/graphics-file-session. The first test verifies that the

Please use double space "  " between sentences. See
https://orgmode.org/worg/org-contribute.html#commit-messages

> -				  (format "print -dpng %s" gfx-file))
> +				  (format "print -dpng %s\nans=%S" gfx-file gfx-file))

Is there any reason why %S but not %s?

>  * Graphical tests
> -#+begin_src octave :results graphics :file chart.png
> +
> +Graphics file. This test is performed by =ob-octave/graphics-file= in =testing/lisp/test-ob-octave.el=.

By convention, we use double space in distributed Org files and ~code~
for symbol markup. See doc/Documentation_Standards.org.

(It is not strictly necessary here, but would be nice to be consistent)

> +          (org-babel-execute-src-block)
> +          (should (search-forward (format "[[file:%s]]" file) nil nil))
> +          (should (file-readable-p file))
> +          (should (let ((size (nth 7 (file-attributes file))))

It would be more clear to use `file-attribute-size' instead of `nth'.

> +                    (> size 0)))
> +          (should (not (get-buffer "*Org-Babel Error Output*"))))

`should-not' would be a bit more succinct.

> +          (should (let ((size (nth 7 (file-attributes file))))
> +                    (> size 0)))
> +          (should (not (get-buffer "*Org-Babel Error Output*"))))

See the previous comment.

-- 
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] 7+ messages in thread

* Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects
  2022-11-09  5:14     ` Ihor Radchenko
@ 2022-11-09 20:33       ` Leo Butler
  2022-11-14  1:56         ` Ihor Radchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Leo Butler @ 2022-11-09 20:33 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Org Mode Mailing List

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

Ihor, see below.

On Wed, Nov 09 2022, Ihor Radchenko <yantar92@posteo.net> wrote:

> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>
>> Ihor,
>> Thanks for your feeback and the pointer. I have revised the tests and
>> attach the revised patch.
>
> Thanks!
>
> Note that your patch is over 15LOC, which exceeds legally allowed
> contribution size for people without copyright assignment.
>
> Would you be interested to sign the copyright assignment form and send
> it to FSF? See https://orgmode.org/worg/org-contribute.html#copyright
> for details. The process usually takes a few days on FSF side (they are
> obliged to reply within 5 working days).

Yes, I have done that. I did the paperwork about 10 years ago, but I
cannot find it and, except for my name, all other details have changed.

>
> Below are some comments.
>
>> * testing/lisp/test-ob-octave.el:
>>
>>   Add the tests ob-octave/graphics-file and
>>   ob-octave/graphics-file-session. The first test verifies that the
>
> Please use double space "  " between sentences. See
> https://orgmode.org/worg/org-contribute.html#commit-messages

Done.

>
>> -				  (format "print -dpng %s" gfx-file))
>> +				  (format "print -dpng %s\nans=%S" gfx-file gfx-file))
>
> Is there any reason why %S but not %s?

That is a good point. Both should be %S. This change is part of the
modified patch (and an extra test).

>
>>  * Graphical tests
>> -#+begin_src octave :results graphics :file chart.png
>> +
>> +Graphics file. This test is performed by =ob-octave/graphics-file= in =testing/lisp/test-ob-octave.el=.
>
> By convention, we use double space in distributed Org files and ~code~
> for symbol markup. See doc/Documentation_Standards.org.
>
> (It is not strictly necessary here, but would be nice to be consistent)
>

Done.

>> +          (org-babel-execute-src-block)
>> +          (should (search-forward (format "[[file:%s]]" file) nil nil))
>> +          (should (file-readable-p file))
>> +          (should (let ((size (nth 7 (file-attributes file))))
>
> It would be more clear to use `file-attribute-size' instead of `nth'.

Thanks. Done.

>
>> +                    (> size 0)))
>> +          (should (not (get-buffer "*Org-Babel Error Output*"))))
>
> `should-not' would be a bit more succinct.

Thanks. Done.

>
>> +          (should (let ((size (nth 7 (file-attributes file))))
>> +                    (> size 0)))
>> +          (should (not (get-buffer "*Org-Babel Error Output*"))))
>
> See the previous comment.

Done.

The amended patch is attached. Thanks for your helpful feedback.

Leo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-prevent-error-in-Octave-process-add-tests-update-tes.patch --]
[-- Type: text/x-diff; name="0001-prevent-error-in-Octave-process-add-tests-update-tes.patch", Size: 6478 bytes --]

From 3d0a083f11a2b4f395c730a04ca243dda4a3a4e3 Mon Sep 17 00:00:00 2001
From: Leo Butler <leo.butler@umanitoba.ca>
Date: Tue, 8 Nov 2022 13:31:47 -0600
Subject: [PATCH] prevent error in Octave process, add tests, update test docs

* lisp/ob-octave.el (org-babel-execute:octave):

  -Ensure that the special Octave variable `ans' is bound when
  GFX-FILE is non-nil.  The glue code in
  ORG-BABEL-OCTAVE-WRAPPER-METHOD causes Octave to exit with a
  non-zero exit code when `ans' is not bound.

  -Change format control string to %S from %s.  Ensure the graphics
  filename is quoted.  If it is not, Octave may create a mis-named
  file or fail entirely.

* testing/examples/ob-octave-test.org:

  Update the Graphical tests section:
  -put in the correct headers;
  -add a remark about where to find each test.

* testing/lisp/test-ob-octave.el:

  Add the three tests ob-octave/graphics-file,
  ob-octave/graphics-file-session and ob-octave/graphics-file-session.
  The first test verifies that the first bug identified above is
  fixed; it also verifies that graphics file creation works correctly
  for scripting.  The second test verifies graphics file creation
  works correctly for sessions.  The third test verifies that a
  graphics filename with a space in it is created correctly.

Thanks to Ihor Radchenko for helpful feedback.
Ref: https://list.orgmode.org/8735asbtfe.fsf@localhost/T/#u
---
 lisp/ob-octave.el                   |  2 +-
 testing/examples/ob-octave-test.org | 12 +++++-
 testing/lisp/test-ob-octave.el      | 64 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-octave.el b/lisp/ob-octave.el
index 55926b789..1cb0e70b5 100644
--- a/lisp/ob-octave.el
+++ b/lisp/ob-octave.el
@@ -91,7 +91,7 @@ end")
 				 (list
 				  "set (0, \"defaultfigurevisible\", \"off\");"
 				  full-body
-				  (format "print -dpng %s" gfx-file))
+				  (format "print -dpng %S\nans=%S" gfx-file gfx-file))
 				 "\n")
 		    full-body)
 		  result-type matlabp)))
diff --git a/testing/examples/ob-octave-test.org b/testing/examples/ob-octave-test.org
index 9839d637e..bc19051a1 100644
--- a/testing/examples/ob-octave-test.org
+++ b/testing/examples/ob-octave-test.org
@@ -46,10 +46,18 @@ ans = s
 
 
 * Graphical tests
-#+begin_src octave :results graphics :file chart.png
+
+Graphics file.  This test is performed by =ob-octave/graphics-file= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :results file graphics :file sombrero.png
+sombrero;
+#+end_src
+
+Graphics file in a session.  This test is performed by =ob-octave/graphics-file-session= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :session :results graphics file :file sombrero.png
 sombrero;
 #+end_src
 
-#+begin_src octave :session
+Graphics file with a space in name.  This test is performed by =ob-octave/graphics-file-space= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :results graphics file :file sombrero hat.png
 sombrero;
 #+end_src
diff --git a/testing/lisp/test-ob-octave.el b/testing/lisp/test-ob-octave.el
index 78ce10214..714998840 100644
--- a/testing/lisp/test-ob-octave.el
+++ b/testing/lisp/test-ob-octave.el
@@ -64,4 +64,68 @@
     (org-babel-next-src-block 5)
     (should (equal nil (org-babel-execute-src-block)))))
 
+(ert-deftest ob-octave/graphics-file ()
+  "Graphics file.  Test that link is correctly inserted and graphics file is created (and not empty).  Clean-up side-effects."
+  ;; In case a prior test left the Error Output buffer hanging around.
+  (when (get-buffer "*Org-Babel Error Output*")
+    (kill-buffer "*Org-Babel Error Output*"))
+  (let ((file (make-temp-file "test-ob-octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :results file graphics :file %s
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (> (file-attribute-size (file-attributes file)) 0))
+          (should-not (get-buffer "*Org-Babel Error Output*")))
+      ;; clean-up
+      (delete-file file)
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+(ert-deftest ob-octave/graphics-file-session ()
+  "Graphics file in a session.  Test that session is started in *Inferior Octave* buffer, link is correctly inserted and graphics file is created (and not empty).  Clean-up side-effects."
+  (let ((file (make-temp-file "test-ob-octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :session :results file graphics :file %s
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (get-buffer "*Inferior Octave*"))
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (> (file-attribute-size (file-attributes file)) 0))
+          (should-not (get-buffer "*Org-Babel Error Output*")))
+      ;; clean-up
+      (delete-file file)
+      (let (kill-buffer-query-functions kill-buffer-hook)
+        (kill-buffer "*Inferior Octave*"))
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+(ert-deftest ob-octave/graphics-file-space ()
+  "Graphics file with a space in filename.  Test that session is started in *Inferior Octave* buffer, link is correctly inserted and graphics file is created (and not empty).  Clean-up side-effects."
+  (let ((file (make-temp-file "test ob octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :results file graphics :file %s
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (> (file-attribute-size (file-attributes file)) 0))
+          (should-not (get-buffer "*Org-Babel Error Output*")))
+      ;; clean-up
+      (delete-file file)
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+
 (provide 'test-ob-octave)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects
  2022-11-09 20:33       ` Leo Butler
@ 2022-11-14  1:56         ` Ihor Radchenko
  2022-11-15 19:43           ` Leo Butler
  0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2022-11-14  1:56 UTC (permalink / raw)
  To: Leo Butler; +Cc: Org Mode Mailing List

Leo Butler <Leo.Butler@umanitoba.ca> writes:

> The amended patch is attached. Thanks for your helpful feedback.

Thanks for the patch!
It looks good, and the tests are passing. However, there is a side effect
leaving testing/examples/octave-workspace file after running the tests.

Can something be done about this?

-- 
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] 7+ messages in thread

* Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects
  2022-11-14  1:56         ` Ihor Radchenko
@ 2022-11-15 19:43           ` Leo Butler
  0 siblings, 0 replies; 7+ messages in thread
From: Leo Butler @ 2022-11-15 19:43 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Org Mode Mailing List

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On Mon, Nov 14 2022, Ihor Radchenko <yantar92@posteo.net> wrote:

> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>
>> The amended patch is attached. Thanks for your helpful feedback.
>
> Thanks for the patch!
> It looks good, and the tests are passing. However, there is a side effect
> leaving testing/examples/octave-workspace file after running the tests.
>
> Can something be done about this?

Yes, sorry about that, I should have caught that while testing the
patch. the amended patch (attached) prevents Octave from dumping a core
file when running the :session test.

Best,
Leo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-prevent-error-in-Octave-process-add-tests-update-tes.patch --]
[-- Type: text/x-diff; name="0001-prevent-error-in-Octave-process-add-tests-update-tes.patch", Size: 6736 bytes --]

From 386c9c2f65730459bdc69b2e0b0b76e22e32dbc9 Mon Sep 17 00:00:00 2001
From: Leo Butler <leo.butler@umanitoba.ca>
Date: Tue, 8 Nov 2022 13:31:47 -0600
Subject: [PATCH] prevent error in Octave process, add tests, update test docs

* lisp/ob-octave.el (org-babel-execute:octave):

  -Ensure that the special Octave variable `ans' is bound when
  GFX-FILE is non-nil.  The glue code in
  ORG-BABEL-OCTAVE-WRAPPER-METHOD causes Octave to exit with a
  non-zero exit code when `ans' is not bound.

  -Change format control string to %S from %s.  Ensure the graphics
  filename is quoted.  If it is not, Octave may create a mis-named
  file or fail entirely.

* testing/examples/ob-octave-test.org:

  Update the Graphical tests section:
  -put in the correct headers;
  -add a remark about where to find each test.

* testing/lisp/test-ob-octave.el:

  Add the three tests ob-octave/graphics-file,
  ob-octave/graphics-file-session and ob-octave/graphics-file-space.

  -ob-octave/graphics-file: The first test verifies that the first bug
  identified above is fixed; it also verifies that graphics file
  creation works correctly for scripting.

  -ob-octave/graphics-file-session: The second test verifies graphics
  file creation works correctly for sessions.  The Octave command
  `crash_dumps_octave_core(0)' is included to prevent the creation of
  a core file (`octave-workspace').

  -ob-octave/graphics-file-space: The third test verifies that a
  graphics filename with a space in it is created correctly.

Thanks to Ihor Radchenko for helpful feedback.
Ref: https://list.orgmode.org/8735asbtfe.fsf@localhost/T/#u
---
 lisp/ob-octave.el                   |  2 +-
 testing/examples/ob-octave-test.org | 12 +++++-
 testing/lisp/test-ob-octave.el      | 65 +++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-octave.el b/lisp/ob-octave.el
index 55926b789..1cb0e70b5 100644
--- a/lisp/ob-octave.el
+++ b/lisp/ob-octave.el
@@ -91,7 +91,7 @@ end")
 				 (list
 				  "set (0, \"defaultfigurevisible\", \"off\");"
 				  full-body
-				  (format "print -dpng %s" gfx-file))
+				  (format "print -dpng %S\nans=%S" gfx-file gfx-file))
 				 "\n")
 		    full-body)
 		  result-type matlabp)))
diff --git a/testing/examples/ob-octave-test.org b/testing/examples/ob-octave-test.org
index 9839d637e..bc19051a1 100644
--- a/testing/examples/ob-octave-test.org
+++ b/testing/examples/ob-octave-test.org
@@ -46,10 +46,18 @@ ans = s
 
 
 * Graphical tests
-#+begin_src octave :results graphics :file chart.png
+
+Graphics file.  This test is performed by =ob-octave/graphics-file= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :results file graphics :file sombrero.png
+sombrero;
+#+end_src
+
+Graphics file in a session.  This test is performed by =ob-octave/graphics-file-session= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :session :results graphics file :file sombrero.png
 sombrero;
 #+end_src
 
-#+begin_src octave :session
+Graphics file with a space in name.  This test is performed by =ob-octave/graphics-file-space= in =testing/lisp/test-ob-octave.el=.
+#+begin_src octave :results graphics file :file sombrero hat.png
 sombrero;
 #+end_src
diff --git a/testing/lisp/test-ob-octave.el b/testing/lisp/test-ob-octave.el
index 78ce10214..57f40d00b 100644
--- a/testing/lisp/test-ob-octave.el
+++ b/testing/lisp/test-ob-octave.el
@@ -64,4 +64,69 @@
     (org-babel-next-src-block 5)
     (should (equal nil (org-babel-execute-src-block)))))
 
+(ert-deftest ob-octave/graphics-file ()
+  "Graphics file.  Test that link is correctly inserted and graphics file is created (and not empty).  Clean-up side-effects."
+  ;; In case a prior test left the Error Output buffer hanging around.
+  (when (get-buffer "*Org-Babel Error Output*")
+    (kill-buffer "*Org-Babel Error Output*"))
+  (let ((file (make-temp-file "test-ob-octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :results file graphics :file %s
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (> (file-attribute-size (file-attributes file)) 0))
+          (should-not (get-buffer "*Org-Babel Error Output*")))
+      ;; clean-up
+      (delete-file file)
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+(ert-deftest ob-octave/graphics-file-session ()
+  "Graphics file in a session.  Test that session is started in *Inferior Octave* buffer, link is correctly inserted and graphics file is created (and not empty).  Clean-up side-effects."
+  (let ((file (make-temp-file "test-ob-octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :session :results file graphics :file %s
+crash_dumps_octave_core(0);
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (get-buffer "*Inferior Octave*"))
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (> (file-attribute-size (file-attributes file)) 0))
+          (should-not (get-buffer "*Org-Babel Error Output*")))
+      ;; clean-up
+      (delete-file file)
+      (let (kill-buffer-query-functions kill-buffer-hook)
+        (kill-buffer "*Inferior Octave*"))
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+(ert-deftest ob-octave/graphics-file-space ()
+  "Graphics file with a space in filename.  Test that session is started in *Inferior Octave* buffer, link is correctly inserted and graphics file is created (and not empty).  Clean-up side-effects."
+  (let ((file (make-temp-file "test ob octave-" nil ".png")))
+    (unwind-protect
+        (org-test-with-temp-text
+	    (format "#+begin_src octave :results file graphics :file %s
+sombrero;
+#+end_src"
+		    file)
+          (org-babel-execute-src-block)
+          (should (search-forward (format "[[file:%s]]" file) nil nil))
+          (should (file-readable-p file))
+          (should (> (file-attribute-size (file-attributes file)) 0))
+          (should-not (get-buffer "*Org-Babel Error Output*")))
+      ;; clean-up
+      (delete-file file)
+      (when (get-buffer "*Org-Babel Error Output*")
+        (kill-buffer "*Org-Babel Error Output*")))))
+
+
 (provide 'test-ob-octave)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-11-15 19:49 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-07 17:03 [PATCH] rfc: using ert-deftest with side-effects Leo Butler
2022-11-08  7:40 ` Ihor Radchenko
2022-11-08 19:55   ` [PATCH] lisp/ob-octave.el, was " Leo Butler
2022-11-09  5:14     ` Ihor Radchenko
2022-11-09 20:33       ` Leo Butler
2022-11-14  1:56         ` Ihor Radchenko
2022-11-15 19:43           ` Leo Butler

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).