Re: [PATCH] Re: Bug: Plain https links with brackets are not recognised [9.4.4 (release_9.4.4-625-g763c7a @ /home/yantar92/.emacs.d/straight/build/org/)]

[Maxim Nikulin <manikulin@gmail.com>]

On 16/03/2021 19:35, Ihor Radchenko wrote:
> +;;; Link regexps
> +
> +(ert-deftest test-ol/plain-link-re ()
> +  "Test `org-link-plain-re'."
> +  (should
> +   (equal
> +    '("https" "//example.com/qwe()")
> +    (org-test-with-temp-text
> +        "(Some text in parenthesis followed by link with brackets <point>https://example.com/qwe())"
> +      (list (org-element-property :type (org-element-link-parser))
> +            (org-element-property :path (org-element-link-parser))))))
> +  (should
> +   (equal
> +    '("https" "//doi.org/10.1016/0160-791x(79)90023-x")
> +    (org-test-with-temp-text
> +        "<point>https://doi.org/10.1016/0160-791x(79)90023-x"
> +      (list (org-element-property :type (org-element-link-parser))
> +            (org-element-property :path (org-element-link-parser))))))

To be clear, I do not ask for any changes. It is great to have some 
tests even in the current form. I just have never tried ert before, so I 
have some questions.

Am I right that just one failure will be reported if a change in the 
regexp causes problems with several test inputs? I am afraid, it is 
rather inconvenient since this tests are rather for quality of 
heuristics than exact requirements. To find proper balance of accuracy 
and speed/regexp complexity, it is better to have all failures at once. 
I am looking up for a feature like EXPECT_EQ (delayed failure) in 
addition to strict ASSERT_EQ in googletest or at least like 
TestCase.subTest in puthon unittest. For this particular case even 
parametrized tests are enough (googletest, pytest).

I have tried implement something similar to illustrate my expectations. 
I think, for experienced lisp programmers the code looks ugly

(defmacro test-ol/deftest-parametrized
     (prefix func &rest cases)
   (declare (indent 2))
   (cons
    #'progn
    (mapcar
     (lambda (case)
       (pcase-let ((`(,id ,docstring ,expect ,arguments) case))
	(let ((test (intern (concat prefix "--" id)))
	      (exp (if (and expect (listp expect)) `'(,@expect) expect))
	      (args (if (listp arguments) arguments (list arguments))))
	  `(ert-deftest ,test ()
	     ,docstring
	     (should (equal ,exp ,(cons func args)))))))
     cases)))

(defun test-ol/match-link-plain (text)
   (and (string-match org-link-plain-re text)
        (mapcar (lambda (i) (match-string-no-properties i text))
	       ;; Currently there is a useless additional group
	       ;; and I think it should cause failure.
	    ;; (number-sequence 1 (- (/ (length (match-data)) 2) 1) 1)
	       '(1 2))))

(test-ol/deftest-parametrized "test-ol/org-link-plain-re"
			      test-ol/match-link-plain
   ("sheme-only" "No match without host or path"
    nil "Secure https: connection")
   ("scheme-slashes" "Questionable case with no host or path"
    ("file" "///") "Use file:/// for local files")
   ("simple-link" "Simple link"
    ("https" "//orgmode.org/") "Link https://orgmode.org/ to Org site")
   ("false-match" "Failure example: unexpected match"
    nil "Valid file:///bin/bash link")
   ("failed-match" "Failure example: host-path is too short to match"
    ("https" "a") "file:a")
   ("a-typo" "Failure example: slashes missed in expectation"
    ("http" "www.gnu.org") "http://www.gnu.org/"))

(ert t)

In my opinion, test report is acceptable

FFF...

F test-ol/org-link-plain-re--a-typo
     Failure example: slashes missed in expectation
     (ert-test-failed
      ((should
        (equal
	'("http" "www.gnu.org")
	(test-ol/match-link-plain "http://www.gnu.org/")))
       :form
       (equal
        ("http" "www.gnu.org")
        ("http" "//www.gnu.org/"))
       :value nil :explanation
       (list-elt 1
		(arrays-of-different-length 11 14 "www.gnu.org" "//www.gnu.org/" 
first-mismatch-at 0))))

F test-ol/org-link-plain-re--failed-match
     Failure example: host-path is too short to match
     (ert-test-failed
      ((should
        (equal
	'("https" "a")
	(test-ol/match-link-plain "file:a")))
       :form
       (equal
        ("https" "a")
        nil)
       :value nil :explanation
       (different-types
        ("https" "a")
        nil)))

F test-ol/org-link-plain-re--false-match
     Failure example: unexpected match
     (ert-test-failed
      ((should
        (equal nil
	      (test-ol/match-link-plain "Valid file:///bin/bash link")))
       :form
       (equal nil
	     ("file" "///bin/bash"))
       :value nil :explanation
       (different-types nil
		       ("file" "///bin/bash"))))

I do not know if it is possible to implement "might" (in addition to 
"should") that using restart or some other technique will prevent 
immediate abandoning of the test but will mark whole test as failed at 
the end.

Actually I hope to get response that I am trying to reinvent a wheel and 
org or emacs has an established way to write tests feeding the same 
function with list of cases and to get all failures in a reasonably 
readable report.