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

[Ihor Radchenko <yantar92@posteo.net>]

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>