bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Sun, 2023-09-10 19:57 +0300
>
>> From: Sebastian Miele <iota@whxvd.name>
>> Cc: Eli Zaretskii <eliz@gnu.org>, Ihor Radchenko <yantar92@posteo.net>
>> Date: Sun, 10 Sep 2023 18:31:20 +0200
>> 
>> I removed emacs-orgmode@gnu.org from CC.
>> 
>> > From: Sebastian Miele <iota@whxvd.name>
>> > Date: Wed, 2023-09-06 15:30 +0200
>> >
>> > I will write the tests.  And I will probably come up with an updated
>> > version of the original patch.  There is at least one cosmetic change.
>> > And something else that I want to have tried.  May take some time.
>> 
>> Please have a look at the following patch.  For now it contains three
>> tests, two of them with :expected-result :failed.  (They do not fail on
>> the bug-fixed version of `kill-whole-line'.)
>
> Yes, there should be more tests, ideally: there are situations where
> kill-whole-line signals an error,

Those tests are on the radar.

> and I don't think I see tests where some of the text is invisible (as
> the function uses forward-visible-line and end-of-visual-line).

That already is covered via (org-mode) and (org-fold-hide-sublevels 1)
in test `kill-whole-line-invisible'.

>> There probably will be more tests and further questions.  But for now, I
>> would like to basically have a statement of whether the style of writing
>> the tests goes in an acceptable direction.
>
> Looks reasonable, but I'm not sure I understand what will the test
> show if one of the tests fails: will the information shown then tell
> enough to understand which of the sub-tests failed and why?

That almost certainly would not be immediately obvious in the current
state.  I have next to no experience in working with testing frameworks.
But I assumed that regressions do not happen that often, and that it
would be good enough if the code of the test can be grasped quickly.
Then, in case of a regression, the test code can be temporarily
sprinkled with some printf-like debugging to find out the exact location
in the test.

However, enough of that printf-like debugging could also be hard-coded,
like in the following definition (see the line ending in the comment
"Provide some context"):

  (ert-deftest kill-whole-line-read-only ()
    ;;:expected-result :failed
    (cl-flet
        ((subtest (kill-whole-line-arg expected-kill-lines 
expected-buffer-lines)
           (should `(subtest ,kill-whole-line-arg)) ; Provide some context
           (ert-with-test-buffer-selected nil
             (simple-tests--set-buffer-text-point-mark
              (string-join '("-2" "-1" "A<POINT>B" "1" "2" "") "\n"))
             (ert-simulate-command '(read-only-mode 1))
             (should-error (ert-simulate-command
                            `(kill-whole-line ,kill-whole-line-arg))
                           :type 'buffer-read-only)
             (should (equal (string-join expected-kill-lines "\n")
                            (car kill-ring)))
             (should (equal (string-join expected-buffer-lines "\n")
                            (simple-tests--get-buffer-text-point-mark))))))
      (subtest 0 '("AB") '("-2" "-1" "AB<POINT>" "1" "2" ""))
      (subtest 1 '("AB" "") '("-2" "-1" "AB" "<POINT>1" "2" ""))
      (subtest 2 '("AB" "1" "") '("-2" "-1" "AB" "1" "<POINT>2" ""))
      (subtest 3 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
      (subtest 9 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
      (subtest -1 '("" "AB") '("-2" "-1<POINT>" "AB" "1" "2" ""))
      (subtest -2 '("" "-1" "AB") '("-2<POINT>" "-1" "AB" "1" "2" ""))
      (subtest -3 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))
      (subtest -9 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))))

With the always succeeding

  (should `(subtest ,kill-whole-line-arg)) ; Provide some context

at the beginning of ervery subtest, the context would be clear after
pressing l in a buffer showing the ERT test results (but never on the
console).

An alternative would be to use `message'.  That would also provide the
context on the console.  However, that also may be a bit noisy.

Another possibility would be to define every subtest as a top-level
test, by a macro like:

  (defmacro simple-test--define-kill-whole-line-read-only-test 
(kill-whole-line-arg)
    ...)

But that feels a bit over the top and unflexible.

What to me feels like an ideal solution would be the concept of a
current context during an ERT test.  Just something like a (defvar
ert-current-context) that always initially is dynamically let-bound to
nil during a test.  That could be setq'ed at different locations in a
test to different arbitrary values (somewhat like ERT explanations).
When a should fails, and the ert-current-context is non-nil, ERT would
display the current context as the first information on the failed test.

WDYT?

For now I assume that providing context via

  (should `(subtest ,kill-whole-line-arg)) ; Provide some context

is good enough.