Re: [O] [PATCH 1/2] Add tests for org-refile-get-targets

[Nicolas Goaziou]

Hello,

Sebastian Reuße  writes:

> Fair enough. Thank you! FWIW, is there some handy table to consult on
> newly supported functions by Emacs version? Or do you have it all
> memorized from the Emacs changelogs?

When I encounter a suspicious function, i.e., a function I have hardly
seen before, I use  an search for it in the document. I also
quickly browse Emacs 24.4 and 24.5 changes with .

That's rather low-level.

Regards,

-- 
Nicolas Goaziou

Re: [O] [PATCH 1/2] Add tests for org-refile-get-targets

[Sebastian Reuße]

Nicolas Goaziou  writes:

> Sebastian Reuße  writes:

>> It didn’t feel right copy-pasting the tests wholesale, so I made a
>> helper-macro. I checked the ert output by forcing a failure and the
>> failure explanation looks as expected. Does this work for you?

> Honestly, I still find it difficult to follow and debug when one the
> tests is failing. Also, we can't use `alist-get' since we are still
> supporting Emacs 24.3.

Fair enough. Thank you! FWIW, is there some handy table to consult on
newly supported functions by Emacs version? Or do you have it all
memorized from the Emacs changelogs?

Kind regards,

-- 
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay

Re: [O] [PATCH 1/2] Add tests for org-refile-get-targets

[Nicolas Goaziou]

Hello,

Sebastian Reuße  writes:

> Sure. Have a look at the follow-up patch and let me know what you
> think.

Thank you!

> It didn’t feel right copy-pasting the tests wholesale, so I made a
> helper-macro. I checked the ert output by forcing a failure and the
> failure explanation looks as expected. Does this work for you?

Honestly, I still find it difficult to follow and debug when one the
tests is failing. Also, we can't use `alist-get' since we are still
supporting Emacs 24.3.

Taking inspiration from your patch, I wrote a test for
`refile-get-targets' in f335c3517de06eb74a3c3727843f276147795a84. It has
more code duplication than yours. OTOH, it doesn't require any
hard-coded file. Hopefully, it will be simpler to debug when a problem
arises in the function.


Regards,

-- 
Nicolas Goaziou

Re: [O] [PATCH 1/2] Add tests for org-refile-get-targets

[Sebastian Reuße]

Hello Nicolas,

Nicolas Goaziou  writes:

> Nitpick: Sections in test-org.el are sorted alphabetically. So the new
> "Refile" section could go between "Radio Targets" and "Sparse trees".

Thank you, I hadn’t noticed.

> Would it be possible to split this big test into smaller ones, with
> a description about what is really tested? See other tests in
> "test-org.el" for some examples. Big tests tend to not being very
> informative when they fail. IMO, code duplication is not an issue in
> test files when it makes tests more readable/useful.

Sure. Have a look at the follow-up patch and let me know what you think.
It didn’t feel right copy-pasting the tests wholesale, so I made a
helper-macro. I checked the ert output by forcing a failure and the
failure explanation looks as expected. Does this work for you?

> It would be even better if you can avoid relying on real files ("a.org"
> and "b.org" in your patch), but if it makes the test too convoluted, no
> worries.

In the follow-up, I stuck with this primarily because of the
‘full-file-path’ test. I figure one could somehow mock a file-backed
buffer, but I expected that would be too involved just to have
everything inlined in the test. Let me know if you think differently
though.

Kind regards,

SR

-- 
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay

Re: [O] [PATCH 1/2] Add tests for org-refile-get-targets

[Nicolas Goaziou]

Hello,

Sebastian Reuße  writes:

> * testing/lisp/test-org.el: Add test.
> ---

Thank you.

>  testing/examples/refile/a.org |  6 ++
> +
> +;;; org-refile

Nitpick: Sections in test-org.el are sorted alphabetically. So the new
"Refile" section could go between "Radio Targets" and "Sparse trees".

> +(ert-deftest test-org/org-refile-get-targets ()
> +  "Test `org-refile-get-targets'."
> +  (save-window-excursion
> +(let ((examples-dir (file-truename "../examples/refile/")))
> +  (cd examples-dir)
> +  (find-file-read-only "a.org")
> +  (find-file-read-only "b.org")
> +  (rename-buffer "gratuitous-prefix/b.org")
> +  (let ((org-refile-targets '((("a.org" "b.org") :level . 2)))
> + (testcases
> +  `((nil . ("a/1/2"
> +"a/2/2"
> +"b/1/2"
> +"b/2/2"))
> +(file . ("a.org"
> + "a.org/a\\/1\\/1/a\\/1\\/2"
> + "a.org/a\\/2\\/1/a\\/2\\/2"
> + "b.org"
> + "b.org/b\\/1\\/1/b\\/1\\/2"
> + "b.org/b\\/2\\/1/b\\/2\\/2"))
> +(full-file-path . ,(mapcar (lambda (s) (concat examples-dir s))
> +   '("a.org"
> + "a.org/a\\/1\\/1/a\\/1\\/2"
> + "a.org/a\\/2\\/1/a\\/2\\/2"
> + "b.org"
> + "b.org/b\\/1\\/1/b\\/1\\/2"
> + "b.org/b\\/2\\/1/b\\/2\\/2")))
> +(buffer-name . ("a.org"
> +"a.org/a\\/1\\/1/a\\/1\\/2"
> +"a.org/a\\/2\\/1/a\\/2\\/2"
> +"gratuitous-prefix/b.org"
> +"gratuitous-prefix/b.org/b\\/1\\/1/b\\/1\\/2"
> +
> "gratuitous-prefix/b.org/b\\/2\\/1/b\\/2\\/2")
> + (cl-loop for (use-outline-path . expected-targets) in testcases
> +  do (let ((org-refile-use-outline-path use-outline-path))
> +   (should
> +(equal
> + (mapcar #'car
> + (org-refile-get-targets))
> + expected-targets
> +

Would it be possible to split this big test into smaller ones, with
a description about what is really tested? See other tests in
"test-org.el" for some examples. Big tests tend to not being very
informative when they fail. IMO, code duplication is not an issue in
test files when it makes tests more readable/useful.

It would be even better if you can avoid relying on real files ("a.org"
and "b.org" in your patch), but if it makes the test too convoluted, no
worries.

Regards,

-- 
Nicolas Goaziou