[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with s
From: |
Ihor Radchenko |
Subject: |
Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects |
Date: |
Wed, 09 Nov 2022 05:14:45 +0000 |
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>
- [PATCH] rfc: using ert-deftest with side-effects, Leo Butler, 2022/11/07
- Re: [PATCH] rfc: using ert-deftest with side-effects, Ihor Radchenko, 2022/11/08
- [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects, Leo Butler, 2022/11/08
- Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects,
Ihor Radchenko <=
- Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects, Leo Butler, 2022/11/09
- Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects, Ihor Radchenko, 2022/11/14
- Re: [PATCH] lisp/ob-octave.el, was [PATCH] rfc: using ert-deftest with side-effects, Leo Butler, 2022/11/15