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

[Leo Butler]

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

0001-prevent-error-in-Octave-process-add-tests-update-tes.patch
Description: 0001-prevent-error-in-Octave-process-add-tests-update-tes.patch