[PATCH] rfc: using ert-deftest with side-effects

[PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

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

Re: [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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>

[PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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>

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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>

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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

> 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

Upon confirming the FSF copyright assignment, I have applied the patch
onto bugfix.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=01c0ebee2

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>
>> 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
>
> Upon confirming the FSF copyright assignment, I have applied the patch
> onto bugfix.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=01c0ebee2

Your patch appears to not work in some environments:

https://builds.sr.ht/~bzg/job/906710

Any ideas?

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> Upon confirming the FSF copyright assignment, I have applied the patch
>> onto bugfix.
>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=01c0ebee2
>
> Your patch appears to not work in some environments:
>
> https://builds.sr.ht/~bzg/job/906710
>
> Any ideas?

Note that the tests are failing only partially. The graphics file does
get created, but it has 0 size for some reason. Maybe something to do
with non-graphical CI environment.

I disabled the tests for the time being until we figure out what is
going on.

https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a29103a78

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

On Wed, Dec 21 2022, Ihor Radchenko <yantar92@posteo.net> wrote:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>>> Upon confirming the FSF copyright assignment, I have applied the patch
>>> onto bugfix.
>>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=01c0ebee2
>>
>> Your patch appears to not work in some environments:
>>
>> https://builds.sr.ht/~bzg/job/906710
>>
>> Any ideas?
>
> Note that the tests are failing only partially. The graphics file does
> get created, but it has 0 size for some reason. Maybe something to do
> with non-graphical CI environment.

There is a race condition between writing the contents of the graphics
file to disk and emacs checking the file size. My guess is that this is
causing the problem (and that the same failure applies for emacs-2{6,7},
since only the emacs-28 reports the exact test failure).

>
> I disabled the tests for the time being until we figure out what is
> going on.
>
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a29103a78

That's pretty crude. Here are a couple thoughts:

- Is there a way to detect that the tests are running in this CI
  environment? We could use that to skip the file-size test selectively.
- Alternatively, we could remove the file-size test entirely.

Either option seems better than not running the tests at all.

Leo

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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

>> Note that the tests are failing only partially. The graphics file does
>> get created, but it has 0 size for some reason. Maybe something to do
>> with non-graphical CI environment.
>
> There is a race condition between writing the contents of the graphics
> file to disk and emacs checking the file size. My guess is that this is
> causing the problem (and that the same failure applies for emacs-2{6,7},
> since only the emacs-28 reports the exact test failure).

Maybe we can just add several `sleep-for' calls to the test?

>> I disabled the tests for the time being until we figure out what is
>> going on.
>>
>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a29103a78
>
> That's pretty crude. Here are a couple thoughts:
>
> - Is there a way to detect that the tests are running in this CI
>   environment? We could use that to skip the file-size test selectively.
> - Alternatively, we could remove the file-size test entirely.

That's temporary solution to not make CI flood me with failure emails
several times a day.

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> There is a race condition between writing the contents of the graphics
>> file to disk and emacs checking the file size. My guess is that this is
>> causing the problem (and that the same failure applies for emacs-2{6,7},
>> since only the emacs-28 reports the exact test failure).
>
> Maybe we can just add several `sleep-for' calls to the test?

I just did this. Let's see if CI errs again.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e5c45358a

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>>> There is a race condition between writing the contents of the graphics
>>> file to disk and emacs checking the file size. My guess is that this is
>>> causing the problem (and that the same failure applies for emacs-2{6,7},
>>> since only the emacs-28 reports the exact test failure).
>>
>> Maybe we can just add several `sleep-for' calls to the test?
>
> I just did this. Let's see if CI errs again.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e5c45358a

Apparently, `sleep-for' 1 second was not enough, and I decided to remove
checking file size completely.

Upon doing this, another failure popped up. This time, it looks like an
actual Elisp issue:

https://builds.sr.ht/~bzg/job/914954
2 unexpected results:
   FAILED  ob-octave/graphics-file  ((should-not (get-buffer "*Org-Babel
   Error Output*")) :form (get-buffer "*Org-Babel Error Output*") :value
   #<killed buffer>) 
   FAILED  ob-octave/graphics-file-space  ((should-not (get-buffer
   "*Org-Babel Error Output*")) :form (get-buffer "*Org-Babel Error
   Output*") :value #<killed buffer>) 

As you can see *Org-Babel Error Output* buffer does not exist when
running the test.

Leo, could you please take a look?

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

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

On Mon, Jan 02 2023, Ihor Radchenko <yantar92@posteo.net> wrote:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Ihor Radchenko <yantar92@posteo.net> writes:
>>
>>>> There is a race condition between writing the contents of the graphics
>>>> file to disk and emacs checking the file size. My guess is that this is
>>>> causing the problem (and that the same failure applies for emacs-2{6,7},
>>>> since only the emacs-28 reports the exact test failure).
>>>
>>> Maybe we can just add several `sleep-for' calls to the test?
>>
>> I just did this. Let's see if CI errs again.
>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e5c45358a
>
> Apparently, `sleep-for' 1 second was not enough, and I decided to remove
> checking file size completely.

Hello Ihor,

Is there an environment variable that could be used to determine is the
tests are being run on sourcehut? This would let us cut out that test on
sourcehut, while still keeping it elsewhere.

>
> Upon doing this, another failure popped up. This time, it looks like an
> actual Elisp issue:
>
> https://builds.sr.ht/~bzg/job/914954
> 2 unexpected results:
>    FAILED  ob-octave/graphics-file  ((should-not (get-buffer "*Org-Babel
>    Error Output*")) :form (get-buffer "*Org-Babel Error Output*") :value
>    #<killed buffer>) 
>    FAILED  ob-octave/graphics-file-space  ((should-not (get-buffer
>    "*Org-Babel Error Output*")) :form (get-buffer "*Org-Babel Error
>    Output*") :value #<killed buffer>) 
>
> As you can see *Org-Babel Error Output* buffer does not exist when
> running the test.
>
> Leo, could you please take a look?

An earlier test is creating that *Org Babel Error Output* buffer. That
is killed on the first test, before the test is actually run. But
GET-BUFFER behaves in an undocumented way: it returns a non-nil value,
#<killed buffer>. To remedy that, I have wrapped the calls in
BUFFER-LIVE-P.

See the attached patch.

Leo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-test-ob-octave.el-wrap-get-buffer-in-buffer-live-p.patch --]
[-- Type: text/x-diff; name="0001-test-ob-octave.el-wrap-get-buffer-in-buffer-live-p.patch", Size: 2172 bytes --]

From b84f2f50b88fe6da3dcca3d751f6d75f7177ddaf Mon Sep 17 00:00:00 2001
From: Leo Butler <leo.butler@umanitoba.ca>
Date: Thu, 5 Jan 2023 13:53:44 -0600
Subject: [PATCH] test-ob-octave.el: wrap get-buffer in buffer-live-p

* testing/lisp/test-ob-octave.el (ob-octave/graphics-file):
(ob-octave/graphics-file-session):
(ob-octave/graphics-file-space): Wrap GET-BUFFER in BUFFER-LIVE-P.
This ensures that a killed buffer does not cause an incorrect failure
of a test.

Link: https://orgmode.org/list/87bknh5nva.fsf@localhost
---
 testing/lisp/test-ob-octave.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/testing/lisp/test-ob-octave.el b/testing/lisp/test-ob-octave.el
index 4e9fea97b..0b8ecea3b 100644
--- a/testing/lisp/test-ob-octave.el
+++ b/testing/lisp/test-ob-octave.el
@@ -79,7 +79,7 @@ sombrero;
           (org-babel-execute-src-block)
           (should (search-forward (format "[[file:%s]]" file) nil nil))
           (should (file-readable-p file))
-          (should-not (get-buffer "*Org-Babel Error Output*")))
+          (should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))))
       ;; clean-up
       (delete-file file)
       (when (get-buffer "*Org-Babel Error Output*")
@@ -99,7 +99,7 @@ sombrero;
           (should (get-buffer "*Inferior Octave*"))
           (should (search-forward (format "[[file:%s]]" file) nil nil))
           (should (file-readable-p file))
-          (should-not (get-buffer "*Org-Babel Error Output*")))
+          (should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))))
       ;; clean-up
       (delete-file file)
       (let (kill-buffer-query-functions kill-buffer-hook)
@@ -119,7 +119,7 @@ sombrero;
           (org-babel-execute-src-block)
           (should (search-forward (format "[[file:%s]]" file) nil nil))
           (should (file-readable-p file))
-          (should-not (get-buffer "*Org-Babel Error Output*")))
+          (should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))))
       ;; clean-up
       (delete-file file)
       (when (get-buffer "*Org-Babel Error Output*")
-- 
2.39.0

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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

>> Apparently, `sleep-for' 1 second was not enough, and I decided to remove
>> checking file size completely.
>
> Hello Ihor,
>
> Is there an environment variable that could be used to determine is the
> tests are being run on sourcehut? This would let us cut out that test on
> sourcehut, while still keeping it elsewhere.

No, we have nothing like this.

In theory, we can bind something in
https://git.sr.ht/~bzg/org-mode-tests/tree/master/item/.builds/init.el,
but I am not sure if it is a good idea.

The tests are failing not because something wrong in the CI machine, but
simply because CI machine is slow. You can get similar issue when
running Org tests on an actual proper old PC or simply when someone is
running CPU-heavy process alongside with Org tests.

So, I do not think that creating exceptions for CI is a good idea.

>> https://builds.sr.ht/~bzg/job/914954
>> 2 unexpected results:
>>    FAILED  ob-octave/graphics-file  ((should-not (get-buffer "*Org-Babel
>>    Error Output*")) :form (get-buffer "*Org-Babel Error Output*") :value
>>    #<killed buffer>) 
>>    FAILED  ob-octave/graphics-file-space  ((should-not (get-buffer
>>    "*Org-Babel Error Output*")) :form (get-buffer "*Org-Babel Error
>>    Output*") :value #<killed buffer>) 
>>
>> As you can see *Org-Babel Error Output* buffer does not exist when
>> running the test.
>>
>> Leo, could you please take a look?
>
> An earlier test is creating that *Org Babel Error Output* buffer. That
> is killed on the first test, before the test is actually run. But
> GET-BUFFER behaves in an undocumented way: it returns a non-nil value,
> #<killed buffer>. To remedy that, I have wrapped the calls in
> BUFFER-LIVE-P.

This is not undocumented. The killed buffers still exists as Elisp
objects:

28.10 Killing Buffers
=====================

“Killing a buffer” makes its name unknown to Emacs and makes the memory
space it occupied available for other use.

   The buffer object for the buffer that has been killed remains in
existence as long as anything refers to it, but it is specially marked
so that you cannot make it current or display it.  Killed buffers retain
their identity, however; if you kill two distinct buffers, they remain
distinct according to ‘eq’ although both are dead.


> See the attached patch.

Thanks!
Installed onto bugfix.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=41ebc2e40

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

On Fri, Jan 06 2023, Ihor Radchenko <yantar92@posteo.net> wrote:

> ********************************************************
> Caution: This message was sent from outside the University of Manitoba.
> ********************************************************
>
> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>
>>> Apparently, `sleep-for' 1 second was not enough, and I decided to remove
>>> checking file size completely.
>>
>> Hello Ihor,
>>
>> Is there an environment variable that could be used to determine is the
>> tests are being run on sourcehut? This would let us cut out that test on
>> sourcehut, while still keeping it elsewhere.
>
> No, we have nothing like this.
>
> In theory, we can bind something in
> https://git.sr.ht/~bzg/org-mode-tests/tree/master/item/.builds/init.el,
> but I am not sure if it is a good idea.
>
> The tests are failing not because something wrong in the CI machine, but
> simply because CI machine is slow. You can get similar issue when
> running Org tests on an actual proper old PC or simply when someone is
> running CPU-heavy process alongside with Org tests.
>
> So, I do not think that creating exceptions for CI is a good idea.

Ok.

>
>>> https://builds.sr.ht/~bzg/job/914954
>>> 2 unexpected results:
>>>    FAILED  ob-octave/graphics-file  ((should-not (get-buffer "*Org-Babel
>>>    Error Output*")) :form (get-buffer "*Org-Babel Error Output*") :value
>>>    #<killed buffer>) 
>>>    FAILED  ob-octave/graphics-file-space  ((should-not (get-buffer
>>>    "*Org-Babel Error Output*")) :form (get-buffer "*Org-Babel Error
>>>    Output*") :value #<killed buffer>) 
>>>
>>> As you can see *Org-Babel Error Output* buffer does not exist when
>>> running the test.
>>>
>>> Leo, could you please take a look?
>>
>> An earlier test is creating that *Org Babel Error Output* buffer.

I will try to look into improving the tests so that we can trap the test(s)
that is(are) creating that error buffer.

>> That is killed on the first test, before the test is actually
>> run. But GET-BUFFER behaves in an undocumented way: it returns a
>> non-nil value, #<killed buffer>. To remedy that, I have wrapped the
>> calls in BUFFER-LIVE-P.
>
> This is not undocumented. The killed buffers still exists as Elisp
> objects:

Thanks, for pointing that out. I was relying on the docstring for
GET-BUFFER. I see that I should have looked at the Elisp
manual. Apologies.

>> See the attached patch.
>
> Thanks!
> Installed onto bugfix.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=41ebc2e40

Regards,
Leo

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> See the attached patch.
>
> Thanks!
> Installed onto bugfix.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=41ebc2e40

And still not enough...
Though this time it does not look like our fault:

https://builds.sr.ht/~bzg/job/918602
3 unexpected results:
   FAILED  ob-octave/graphics-file  ((should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))) :form (buffer-live-p #<killed buffer>) :value t)
   FAILED  ob-octave/graphics-file-space  ((should-not (buffer-live-p
   (get-buffer "*Org-Babel Error Output*"))) :form (buffer-live-p
   #<killed buffer>) :value t)

I filed a bug report to Emacs:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60626

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

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

On Sat, Jan 07 2023, Leo Butler <Leo.Butler@umanitoba.ca> wrote:

> On Fri, Jan 06 2023, Ihor Radchenko <yantar92@posteo.net> wrote:
>
>>
>> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>>
>>>> https://builds.sr.ht/~bzg/job/914954
>>>> 2 unexpected results:
>>>>    FAILED  ob-octave/graphics-file  ((should-not (get-buffer "*Org-Babel
>>>>    Error Output*")) :form (get-buffer "*Org-Babel Error Output*") :value
>>>>    #<killed buffer>) 
>>>>    FAILED  ob-octave/graphics-file-space  ((should-not (get-buffer
>>>>    "*Org-Babel Error Output*")) :form (get-buffer "*Org-Babel Error
>>>>    Output*") :value #<killed buffer>) 
>>>>
>>>> As you can see *Org-Babel Error Output* buffer does not exist when
>>>> running the test.
>>>>
>>>> Leo, could you please take a look?
>>>
>>> An earlier test is creating that *Org Babel Error Output* buffer.
>
> I will try to look into improving the tests so that we can trap the test(s)
> that is(are) creating that error buffer.

See the attachment. There are four test failures that are currently
untrapped. I also see the `buffer-live-p' bug. 

Leo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org-tests.org --]
[-- Type: text/x-org; name="org-tests.org", Size: 14725 bytes --]

#+AUTHOR: Leo Butler
#+DATE: 10 Jan 2023
#+TITLE: False positives in ~org~ tests
#+STARTUP: show2levels

* Summary
Applying the [[patch-to--org-test-with-temp-text]] reveals 4 tests that fail, but which are passed with the unpatched ~org-test-with-temp-text~ macro.
The next sections show the test failures. References:
- *Org-Babel Error Output* :: [[https://orgmode.org/list/87bknh5nva.fsf@localhosthttps://orgmode.org/list/87bknh5nva.fsf@localhost]]
- buffer-live-p :: [[https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60626]]
  
#+name: patch-to--org-test-with-temp-text
#+begin_example
diff --git a/testing/org-test.el b/testing/org-test.el
index 22ac60670..7909f36cc 100644
--- a/testing/org-test.el
+++ b/testing/org-test.el
@@ -196,7 +196,18 @@ otherwise place the point at the beginning of the inserted text."
 	   (insert inside-text)
 	   (goto-char (point-min))))
        (font-lock-ensure (point-min) (point-max))
-       ,@body)))
+       (prog1
+           (progn ,@body)
+         (unwind-protect
+             (should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*")))
+           (when (get-buffer "*Org-Babel Error Output*")
+             (message "Detected: *Org-Babel Error Output*")
+             (message "Contents:")
+             (message (with-current-buffer "*Org-Babel Error Output*"
+                        (buffer-substring-no-properties (point-min) (point-max))))
+             (message "End:")
+             (kill-buffer "*Org-Babel Error Output*"))))
+         )))
 
 (defmacro org-test-with-temp-text-in-file (text &rest body)
   "Run body in a temporary file buffer with Org mode as the active mode.
   #+end_example
* Command
The tests are run in-place like so:
#+begin_src sh :exports none :results raw drawer
make test-dirty
#+end_src

* Test Failures
** Failure 1
#+begin_example
Detected: *Org-Babel Error Output*
Contents:
/tmp/tmp-orgtest/fortran-src-ym6m0X.F90:5:17:

    5 | write (*, ’(i2)’), nint(s(1,2))
      |                 1
Warning: Legacy Extension: Comma before i/o item list at (1)
[ Babel evaluation exited with code 0 ]
/tmp/tmp-orgtest/fortran-src-EYVgCk.F90:5:17:

    5 | write (*, ’(i2)’), nint(s(2,3))
      |                 1
Warning: Legacy Extension: Comma before i/o item list at (1)
[ Babel evaluation exited with code 0 ]
/tmp/tmp-orgtest/fortran-src-SwTuHS.F90:5:20:

    5 | write (*, ’(3f5.2)’), s
      |                    1
Warning: Legacy Extension: Comma before i/o item list at (1)
[ Babel evaluation exited with code 0 ]
/tmp/tmp-orgtest/fortran-src-xnH1cO.F90:5:20:

    5 | write (*, ’(2f5.2)’), s
      |                    1
Warning: Legacy Extension: Comma before i/o item list at (1)
[ Babel evaluation exited with code 0 ]
End:
Test ob-java/args-quoted-string backtrace:
  ert-fail(((should-not (buffer-live-p (get-buffer "*Org-Babel Error O
  (if (not (unwind-protect (setq value-1015 (apply fn-1013 args-1014))
  (let (form-description-1017) (if (not (unwind-protect (setq value-10
  (let ((value-1015 'ert-form-evaluation-aborted-1016)) (let (form-des
  (let* ((fn-1013 #'buffer-live-p) (args-1014 (condition-case err (let
  (unwind-protect (let* ((fn-1013 #'buffer-live-p) (args-1014 (conditi
  (prog1 (progn (let* ((fn-1008 #'string=) (args-1009 (condition-case 
  (progn (org-mode) (let ((point (string-match "<point>" inside-text))
  (unwind-protect (progn (org-mode) (let ((point (string-match "<point
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (let ((inside-text (if (stringp "#+begin_src java :dir 'nil :results
  (let ((lexical-binding t)) (let ((inside-text (if (stringp "#+begin_
  (closure (t) nil (let ((lexical-binding t)) (let ((inside-text (if (
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name ob-java/args-quoted-string :documenta
  ert-run-or-rerun-test(#s(ert--stats :selector "\\(org\\|ob\\|ox\\...
  ert-run-tests("\\(org\\|ob\\|ox\\)" #f(compiled-function (event-type
  ert-run-tests-batch("\\(org\\|ob\\|ox\\)")
  ert-run-tests-batch-and-exit("\\(org\\|ob\\|ox\\)")
  (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
  org-test-run-batch-tests("\\(org\\|ob\\|ox\\)")
  command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
  command-line()
  normal-top-level()
Test ob-java/args-quoted-string condition:
    (ert-test-failed
     ((should-not
       (buffer-live-p
	(get-buffer "*Org-Babel Error Output*")))
      :form
      (buffer-live-p #<killed buffer>)
      :value t))
   FAILED    71/1116  ob-java/args-quoted-string (0.313545 sec)
#+end_example
** Failure 2
#+begin_example
Detected: *Org-Babel Error Output*
Contents:
[ Babel evaluation exited with code 1 ]
[ Babel evaluation exited with code 2 ]
End:
Test ob-shell/remote-with-stdin-or-cmdline backtrace:
  ert-fail(((should-not (buffer-live-p (get-buffer "*Org-Babel Error O
  (if (not (unwind-protect (setq value-2299 (apply fn-2297 args-2298))
  (let (form-description-2301) (if (not (unwind-protect (setq value-22
  (let ((value-2299 'ert-form-evaluation-aborted-2300)) (let (form-des
  (let* ((fn-2297 #'buffer-live-p) (args-2298 (condition-case err (let
  (unwind-protect (let* ((fn-2297 #'buffer-live-p) (args-2298 (conditi
  (prog1 (progn (org-trim (org-babel-execute-src-block))) (unwind-prot
  (progn (org-mode) (let ((point (string-match "<point>" inside-text))
  (unwind-protect (progn (org-mode) (let ((point (string-match "<point
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (let ((inside-text (if (stringp (mapconcat #'identity (list "#+name:
  (let* ((result (let ((inside-text (if (stringp ...) (mapconcat ... .
  (let ((default-directory (or (plist-get spec :dir) default-directory
  (let ((spec (car --dolist-tail--))) (let ((default-directory (or (pl
  (while --dolist-tail-- (let ((spec (car --dolist-tail--))) (let ((de
  (let ((--dolist-tail-- (cons nil (cons (list ':dir remote-dir) (cons
  (closure (t) (remote-dir) (let ((--dolist-tail-- (cons nil (cons (li
  funcall((closure (t) (remote-dir) (let ((--dolist-tail-- (cons nil (
  (let ((tramp-methods (cons '("mock" (tramp-login-program "sh") (tram
  (cond (env-def (funcall body env-def)) ((eq system-type 'windows-nt)
  (let ((env-def (getenv "REMOTE_TEMPORARY_FILE_DIRECTORY"))) (cond (e
  org-test-with-tramp-remote-dir--worker((closure (t) (remote-dir) (le
  (let ((lexical-binding t)) (let* ((fn-2292 #'not) (args-2293 (condit
  (closure (t) nil (let ((lexical-binding t)) (let* ((fn-2292 #'not) (
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name ob-shell/remote-with-stdin-or-cmdline
  ert-run-or-rerun-test(#s(ert--stats :selector "\\(org\\|ob\\|ox\\...
  ert-run-tests("\\(org\\|ob\\|ox\\)" #f(compiled-function (event-type
  ert-run-tests-batch("\\(org\\|ob\\|ox\\)")
  ert-run-tests-batch-and-exit("\\(org\\|ob\\|ox\\)")
  (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
  org-test-run-batch-tests("\\(org\\|ob\\|ox\\)")
  command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
  command-line()
  normal-top-level()
Test ob-shell/remote-with-stdin-or-cmdline condition:
    (ert-test-failed
     ((should-not
       (buffer-live-p
	(get-buffer "*Org-Babel Error Output*")))
      :form
      (buffer-live-p #<killed buffer>)
      :value t))
   FAILED   190/1116  ob-shell/remote-with-stdin-or-cmdline (0.013323 sec)
#+end_example
** Failure 3
#+begin_example
Detected: *Org-Babel Error Output*
Contents:
[ Babel evaluation exited with code 2 ]
End:
Test ob-tangle/jump-to-org backtrace:
  ert--should-signal-hook(ert-test-failed (((should-not (buffer-live-p
  ert-fail(((should-not (buffer-live-p (get-buffer "*Org-Babel Error O
  (if (not (unwind-protect (setq value-2414 (apply fn-2412 args-2413))
  (let (form-description-2416) (if (not (unwind-protect (setq value-24
  (let ((value-2414 'ert-form-evaluation-aborted-2415)) (let (form-des
  (let* ((fn-2412 #'buffer-live-p) (args-2413 (condition-case err (let
  (unwind-protect (let* ((fn-2412 #'buffer-live-p) (args-2413 (conditi
  (prog1 (progn (org-babel-tangle-jump-to-org) (buffer-string)) (unwin
  (progn (org-mode) (let ((point (string-match "<point>" inside-text))
  (unwind-protect (progn (org-mode) (let ((point (string-match "<point
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (let ((inside-text (if (stringp (format ";; [[file:%s][H:1]]\n<point
  (let ((file (buffer-file-name))) (let ((inside-text (if (stringp (fo
  (progn (let ((file (buffer-file-name))) (let ((inside-text (if (stri
  (progn (setq buffer (find-file file)) (if (re-search-forward "<point
  (unwind-protect (progn (setq buffer (find-file file)) (if (re-search
  (let ((file (make-temp-file "org-test")) (inside-text (if (stringp "
  (list "* H\n#+begin_src emacs-lisp\n1\n#+end_src" (let ((file (make-
  (let ((signal-hook-function #'ert--should-signal-hook)) (list "* H\n
  (condition-case err (let ((signal-hook-function #'ert--should-signal
  (let* ((fn-2417 #'equal) (args-2418 (condition-case err (let ((signa
  (let ((org-file-apps '((t . emacs)))) (let* ((fn-2417 #'equal) (args
  (let ((lexical-binding t)) (let ((org-file-apps '((t . emacs)))) (le
  (closure (t) nil (let ((lexical-binding t)) (let ((org-file-apps '((
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name ob-tangle/jump-to-org :documentation 
  ert-run-or-rerun-test(#s(ert--stats :selector "\\(org\\|ob\\|ox\\...
  ert-run-tests("\\(org\\|ob\\|ox\\)" #f(compiled-function (event-type
  ert-run-tests-batch("\\(org\\|ob\\|ox\\)")
  ert-run-tests-batch-and-exit("\\(org\\|ob\\|ox\\)")
  (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
  org-test-run-batch-tests("\\(org\\|ob\\|ox\\)")
  command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
  command-line()
  normal-top-level()
Test ob-tangle/jump-to-org condition:
    (ert-test-failed
     ((should-not
       (buffer-live-p
	(get-buffer "*Org-Babel Error Output*")))
      :form
      (buffer-live-p #<killed buffer>)
      :value t))
   FAILED   206/1116  ob-tangle/jump-to-org (0.007505 sec)
#+end_example
** Failure 4
#+begin_example
Detected: *Org-Babel Error Output*
Contents:
ls: cannot access ’NoSuchFileOrDirectory.txt’: No such file or directory
[ Babel evaluation exited with code 2 ]
End:
Test test-ob/allow-spaces-around-=-in-var-specs backtrace:
  ert-fail(((should-not (buffer-live-p (get-buffer "*Org-Babel Error O
  (if (not (unwind-protect (setq value-3233 (apply fn-3231 args-3232))
  (let (form-description-3235) (if (not (unwind-protect (setq value-32
  (let ((value-3233 'ert-form-evaluation-aborted-3234)) (let (form-des
  (let* ((fn-3231 #'buffer-live-p) (args-3232 (condition-case err (let
  (unwind-protect (let* ((fn-3231 #'buffer-live-p) (args-3232 (conditi
  (prog1 (progn (let* ((fn-3226 #'=) (args-3227 (condition-case err (l
  (progn (org-mode) (let ((point (string-match "<point>" inside-text))
  (unwind-protect (progn (org-mode) (let ((point (string-match "<point
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (let ((inside-text (if (stringp "#+begin_src emacs-lisp :var a = 1 b
  (let ((lexical-binding t)) (let ((inside-text (if (stringp "#+begin_
  (closure (t) nil (let ((lexical-binding t)) (let ((inside-text (if (
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name test-ob/allow-spaces-around-=-in-var-
  ert-run-or-rerun-test(#s(ert--stats :selector "\\(org\\|ob\\|ox\\...
  ert-run-tests("\\(org\\|ob\\|ox\\)" #f(compiled-function (event-type
  ert-run-tests-batch("\\(org\\|ob\\|ox\\)")
  ert-run-tests-batch-and-exit("\\(org\\|ob\\|ox\\)")
  (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
  org-test-run-batch-tests("\\(org\\|ob\\|ox\\)")
  command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
  command-line()
  normal-top-level()
Test test-ob/allow-spaces-around-=-in-var-specs condition:
    (ert-test-failed
     ((should-not
       (buffer-live-p
	(get-buffer "*Org-Babel Error Output*")))
      :form
      (buffer-live-p #<killed buffer>)
      :value t))
   FAILED   315/1116  test-ob/allow-spaces-around-=-in-var-specs (0.003319 sec)
#+end_example
** Test Summary
#+begin_example
Ran 1116 tests, 1102 results as expected, 4 unexpected, 10 skipped (2023-01-10 11:36:40-0600, 64.394581 sec)
2 expected failures

4 unexpected results:
   FAILED  ob-java/args-quoted-string  ((should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))) :form (buffer-live-p #<killed buffer>) :value t)
   FAILED  ob-shell/remote-with-stdin-or-cmdline  ((should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))) :form (buffer-live-p #<killed buffer>) :value t)
   FAILED  ob-tangle/jump-to-org  ((should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))) :form (buffer-live-p #<killed buffer>) :value t)
   FAILED  test-ob/allow-spaces-around-=-in-var-specs  ((should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*"))) :form (buffer-live-p #<killed buffer>) :value t)

10 skipped results:
  SKIPPED  org-missing-dependency/test-ob-R  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-clojure  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-eshell  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-julia  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-lua  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-ruby  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-scheme  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-sql  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-ob-sqlite  ((skip-unless nil) :form nil :value nil)
  SKIPPED  org-missing-dependency/test-org-attach-git  ((skip-unless nil) :form nil :value nil)

make: *** [mk/targets.mk:100: test-dirty] Error 1
#+end_example

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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

>>>>> Leo, could you please take a look?
>>>>
>>>> An earlier test is creating that *Org Babel Error Output* buffer.
>>
>> I will try to look into improving the tests so that we can trap the test(s)
>> that is(are) creating that error buffer.
>
> See the attachment. There are four test failures that are currently
> untrapped. I also see the `buffer-live-p' bug. 

It looks like `buffer-live-p' is not a bug, but rather the result of
backtrace being printed upon executing `kill-buffer' in unwind-protect
form from the test body: (1) test fails; (2) unwind-protect executes
kill-buffer; (3) backtrace is printed with "killed" buffer object.

So, the test failure is real.

https://orgmode.org/list/94980226-D29A-4969-8640-1143A1979164@bundesbrandschatzamt.de
might be related.

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

On Wed, Jan 11 2023, Ihor Radchenko <yantar92@posteo.net> wrote:

> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>
>>>>>> Leo, could you please take a look?
>>>>>
>>>>> An earlier test is creating that *Org Babel Error Output* buffer.
>>>
>>> I will try to look into improving the tests so that we can trap the test(s)
>>> that is(are) creating that error buffer.
>>
>> See the attachment. There are four test failures that are currently
>> untrapped. I also see the `buffer-live-p' bug. 
>
> It looks like `buffer-live-p' is not a bug, but rather the result of
> backtrace being printed upon executing `kill-buffer' in unwind-protect
> form from the test body: (1) test fails; (2) unwind-protect executes
> kill-buffer; (3) backtrace is printed with "killed" buffer object.

Yes, that seems reasonable.

>
> So, the test failure is real.
>
> https://orgmode.org/list/94980226-D29A-4969-8640-1143A1979164@bundesbrandschatzamt.de
> might be related.

Ihor,

How do you want to treat the patch that was included? I think we should
have something like that to catch errors like these. And the failing
tests should be marked as known failures (that need to be fixed,
obviously). I note that both failures 3 & 4 are related to org's
built-in features.

Leo

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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

>> So, the test failure is real.
>>
>> https://orgmode.org/list/94980226-D29A-4969-8640-1143A1979164@bundesbrandschatzamt.de
>> might be related.
>
> Ihor,
>
> How do you want to treat the patch that was included? I think we should
> have something like that to catch errors like these. And the failing
> tests should be marked as known failures (that need to be fixed,
> obviously). I note that both failures 3 & 4 are related to org's
> built-in features.

Are you referring to "failures" in your attachment?
I think that your expectation is wrong here. Org does not clear the
"*Org-Babel Error Output*" buffer upon running next src block during
Emacs session - it is a good thing when you run multiple src blocks and
want to see the errors/warnings produced.

So, if we want to do some definitive assertions about "*Org-Babel Error
Output*", we could kill "*Org-Babel Error Output*" (if any) before
running the test that checks this buffer.

As for ob-octave failures, I suspect that octave throws some warnings on
CI machine. I currently don't have podman setup to run CI tests locally
- you may try to play with the problematic ob-octave tests to identify
the problem as described in https://git.sr.ht/~bzg/org-mode-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>

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> So, the test failure is real.

The error buffer contents when the test fails is the following:

warning: using the gnuplot graphics toolkit is discouraged

The gnuplot graphics toolkit is not actively maintained and has a number
of limitations that are unlikely to be fixed.  Communication with gnuplot
uses a one-directional pipe and limited information is passed back to the
Octave interpreter so most changes made interactively in the plot window
will not be reflected in the graphics properties managed by Octave.  For
example, if the plot window is closed with a mouse click, Octave will not
be notified and will not update its internal list of open figure windows.
The qt toolkit is recommended instead.
         line 0: warning: iconv failed to convert degree sign
error: ignoring const execution_exception& while preparing to exit
[ Babel evaluation exited with code 0 ]

Exit code is 0, so octave does finish.

Hence, test assertion that
(should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*")))
does not appear to be accurate.

Leo, should we simply remove the assertion?

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

On Fri, Jan 13 2023, Ihor Radchenko <yantar92@posteo.net> wrote:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> So, the test failure is real.
>
> The error buffer contents when the test fails is the following:
>
> warning: using the gnuplot graphics toolkit is discouraged
>
> The gnuplot graphics toolkit is not actively maintained and has a number
> of limitations that are unlikely to be fixed.  Communication with gnuplot
> uses a one-directional pipe and limited information is passed back to the
> Octave interpreter so most changes made interactively in the plot window
> will not be reflected in the graphics properties managed by Octave.  For
> example, if the plot window is closed with a mouse click, Octave will not
> be notified and will not update its internal list of open figure windows.
> The qt toolkit is recommended instead.

This is not being caused by a faulty test.

The default graphics toolkit for octave is not gnuplot. To get that
warning, the graphics toolkit needs to be set to gnuplot. But, that is
not done in those tests, so where/how is that happening?

>          line 0: warning: iconv failed to convert degree sign

This warning makes no sense to me.

> error: ignoring const execution_exception& while preparing to exit
> [ Babel evaluation exited with code 0 ]
>
> Exit code is 0, so octave does finish.
>
> Hence, test assertion that
> (should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*")))
> does not appear to be accurate.

I guess it depends on what is meant by `accurate'. Those warnings make
me think that the test is running in a mis-configured environment.

>
> Leo, should we simply remove the assertion?

Ok.

I would really like to understand why the two tests are failing on
sourcehut (but not the one that creates a session), but I can understand
your viewpoint.

Leo

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

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

>> The gnuplot graphics toolkit is not actively maintained and has a number
>> of limitations that are unlikely to be fixed.  Communication with gnuplot
>> uses a one-directional pipe and limited information is passed back to the
>> Octave interpreter so most changes made interactively in the plot window
>> will not be reflected in the graphics properties managed by Octave.  For
>> example, if the plot window is closed with a mouse click, Octave will not
>> be notified and will not update its internal list of open figure windows.
>> The qt toolkit is recommended instead.
>
> This is not being caused by a faulty test.
>
> The default graphics toolkit for octave is not gnuplot. To get that
> warning, the graphics toolkit needs to be set to gnuplot. But, that is
> not done in those tests, so where/how is that happening?

CI machine is running Debian. AFAIU, gnuplot toolkit is default in
octave distributed with Debian.

We do not do anything special wrt configuration:
https://builds.sr.ht/api/jobs/921769/manifest

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Max Nikulin]

On 14/01/2023 20:04, Leo Butler wrote:
>>           line 0: warning: iconv failed to convert degree sign
> This warning makes no sense to me.

Just a guess. Some code creates a plot with the "°" character, but 
worker locale is ASCII, not UTF-8 (en_US.UTF-8, C.UTF-8, etc.)

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> So, the test failure is real.
>
> The error buffer contents when the test fails is the following:
>
> warning: using the gnuplot graphics toolkit is discouraged
> ...
> [ Babel evaluation exited with code 0 ]
>
> Exit code is 0, so octave does finish.
>
> Hence, test assertion that
> (should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*")))
> does not appear to be accurate.
>
> Leo, should we simply remove the assertion?

I decided to remove the assertion from the tests.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=59228e51345ab522d9db611c8e73caa078d86d2f

While there could be a value in making sure that ob-octave calls octave
without generating errors, it is not strictly what the test is checking
for.

Moreover, we currently have no reliable way to disambiguate mere
warnings from non-zero exit code.

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

Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects

[Leo Butler]

On Mon, Jan 23 2023, Ihor Radchenko <yantar92@posteo.net> wrote:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Ihor Radchenko <yantar92@posteo.net> writes:
>>
>>> So, the test failure is real.
>>
>> The error buffer contents when the test fails is the following:
>>
>> warning: using the gnuplot graphics toolkit is discouraged
>> ...
>> [ Babel evaluation exited with code 0 ]
>>
>> Exit code is 0, so octave does finish.
>>
>> Hence, test assertion that
>> (should-not (buffer-live-p (get-buffer "*Org-Babel Error Output*")))
>> does not appear to be accurate.
>>
>> Leo, should we simply remove the assertion?
>
> I decided to remove the assertion from the tests.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=59228e51345ab522d9db611c8e73caa078d86d2f
>
> While there could be a value in making sure that ob-octave calls octave
> without generating errors, it is not strictly what the test is checking
> for.
>
> Moreover, we currently have no reliable way to disambiguate mere
> warnings from non-zero exit code.

Yes, as I said in a previous email, I can live with that.

The origin of the errors that you documented is not, I believe, due to
either of the causes suggested by either Max or you. But, investigating
obscure test failures like this is probably not the best expenditure of
time and effort.

Best regards,
Leo