Ihor, see below. On Wed, Nov 09 2022, Ihor Radchenko <yanta...@posteo.net> wrote:
> Leo Butler <leo.but...@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
From 3d0a083f11a2b4f395c730a04ca243dda4a3a4e3 Mon Sep 17 00:00:00 2001 From: Leo Butler <leo.but...@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
