Broken org-protocol tests

Broken org-protocol tests

[Maxim Nikulin]

Hi,

Is there any reason why org-protocol tests are not run by default? Is 
dependency on server.el considered as too heavy for routine testing? I 
know, the tests are broken, but I suspect, it is a consequence that 
additional efforts are required to run them.

Almost unrelated note: either I missed something or org tests depend on 
a couple of things unavailable in emacs-25, e.g. 
temporary-file-directory function.

[PATCH 1/3] test-org-protocol.el: Fix the case for parse parameters with complex argument

[Maxim Nikulin]

* testing/lisp/test-org-protocol.el
(test-org-protocol/org-protocol-parse-parameters): Specify that the case
simulating real life capture uses new style parameters string
to prevent test failure.

It looks like a typo survived since addition of this case in 2216f4d2c7.
---
 testing/lisp/test-org-protocol.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testing/lisp/test-org-protocol.el b/testing/lisp/test-org-protocol.el
index b5df7c132..5ab96fdc2 100644
--- a/testing/lisp/test-org-protocol.el
+++ b/testing/lisp/test-org-protocol.el
@@ -40,7 +40,7 @@
 		      "url=https%3A%2F%2Forgmode.org%2Forg.html%23capture-protocol&"
 		      "title=The%20Org%20Manual&"
 		      "body=9.4.2%20capture%20protocol"))
-	 (data (org-protocol-parse-parameters url)))
+	 (data (org-protocol-parse-parameters url t)))
     (should (string= (plist-get data :template) "p"))
     (should (string= (plist-get data :url) "https://orgmode.org/org.html#capture-protocol"))
     (should (string= (plist-get data :title) "The Org Manual"))
-- 
2.25.1

Re: [PATCH 1/3] test-org-protocol.el: Fix the case for parse parameters with complex argument

[Bastien]

Applied in maint, thanks and sorry for the slow reply.

Maxim Nikulin <manikulin@gmail.com> writes:

> * testing/lisp/test-org-protocol.el
> (test-org-protocol/org-protocol-parse-parameters): Specify that the case
> simulating real life capture uses new style parameters string
> to prevent test failure.
>
> It looks like a typo survived since addition of this case in
> 2216f4d2c7.

-- 
 Bastien

[PATCH 2/3] org-protocol.el: Fix detection of old-style URIs

[Maxim Nikulin]

* lisp/org-protocol.el (org-protocol-check-filename-for-protocol): Avoid
incorrect regexp in check whether command line argument uses new syntax.
Fix failures of org-protocol tests.

Question mark was not escaped in the previous version 928e67df7e,
so any string was matched by lazy "*".  Match in never used,
thus `string-match-p` would be better, but actually regexp is redundant
here.

It is not documented what browser or desktop environment adds extra
slash before "?".  Accordingly to
mid:A2B0655F-BF28-4943-BC05-99021BFDA1B3@robewald.de, Windows may be
involved.  Likely it happens with double slash after schema as in
org-protocol://capture?url=URL&title=TITLE due to subprotocol is
considered as host name and URI is normalized by adding a slash
as mandatory path part before "?" query.  So just reverting the original
commit will likely cause a regression.  Another guess is that
with single or triple slash (org-protocol:/capture?url=URL)
subprotocol is a part of path thus no "smart" actions are necessary.
---
 lisp/org-protocol.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org-protocol.el b/lisp/org-protocol.el
index 731f51e19..86a8dc3bc 100644
--- a/lisp/org-protocol.el
+++ b/lisp/org-protocol.el
@@ -631,7 +631,7 @@ CLIENT is ignored."
                        (greedy (plist-get (cdr prolist) :greedy))
                        (split (split-string fname proto))
                        (result (if greedy restoffiles (cadr split)))
-		       (new-style (string-match "/*?" (match-string 1 fname))))
+		       (new-style (not (= ?: (aref (match-string 1 fname) 0)))))
                   (when (plist-get (cdr prolist) :kill-client)
 		    (message "Greedy org-protocol handler.  Killing client.")
 		    (server-edit))
-- 
2.25.1

Re: [PATCH 2/3] org-protocol.el: Fix detection of old-style URIs

[Bastien]

Applied in maint, thanks!

-- 
 Bastien

[PATCH 3/3] test-org-protocol.el: Add expected failures for file:/// URLs

[Maxim Nikulin]

* testing/lisp/test-org-protocol.el
(test-org-protocol/org-protocol-store-link-file,
test-org-protocol/org-protocol-capture-file): Add tests to document
that existing calls to `org-protocol-sanitize-uri' could make passed
URLs invalid by changing number of slashes after scheme.

Till a fix of the problem the new tests are marked as expected failures.

Unless a relatively recent commit 5748615c48, I would believe that
`org-protocol-sanitize-uri' was added by mistake to the initial
implementation.  Capture URLs are anyway escaped with percent encoding.
---
 testing/lisp/test-org-protocol.el | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/testing/lisp/test-org-protocol.el b/testing/lisp/test-org-protocol.el
index 5ab96fdc2..d33052b30 100644
--- a/testing/lisp/test-org-protocol.el
+++ b/testing/lisp/test-org-protocol.el
@@ -78,6 +78,16 @@
     (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil)))
     (should (equal (car org-stored-links) '("URL3" "TITLE3")))))
 
+(ert-deftest test-org-protocol/org-protocol-store-link-file ()
+  "store-link: `org-protocol-sanitize-uri' could distort URL."
+  :expected-result :failed
+  (let ((uri "/org-protocol:/store-link:/file%3A%2F%2F%2Fetc%2Fmailcap/Triple%20Slash"))
+    (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil)))
+    (should (equal (car org-stored-links) '("file:///etc/mailcap" "Triple Slash"))))
+  (let ((uri "/org-protocol:/store-link?url=file%3A%2F%2F%2Fetc%2Fmailcap&title=Triple%20Slash"))
+    (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil)))
+    (should (equal (car org-stored-links) '("file:///etc/mailcap" "Triple Slash")))))
+
 (ert-deftest test-org-protocol/org-protocol-capture ()
   "Test `org-protocol-capture' specifications."
   (let* ((org-protocol-default-template-key "t")
@@ -134,6 +144,20 @@
      test-urls)
     (delete-file temp-file-name)))
 
+(ert-deftest test-org-protocol/org-protocol-capture-file ()
+  "capture: `org-protocol-sanitize-uri' could distort URL."
+  :expected-result :failed
+  (let* ((org-protocol-default-template-key "t")
+	 (temp-file-name (make-temp-file "org-protocol-test"))
+	 (org-capture-templates
+	  `(("t" "Test" plain (file ,temp-file-name) "%a\n%i\n" :kill-buffer t))))
+    (let ((uri "/org-protocol:/capture:/t/file%3A%2F%2F%2Fetc%2Fmailcap/Triple%20Slash/Body"))
+      (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil)))
+      (should (string= (buffer-string) "[[file:///etc/mailcap][Triple Slash]]\nBody")))
+    (let ((uri "/org-protocol:/capture?template=t&url=file%3A%2F%2F%2Fetc%2Fmailcap&title=Triple%20Slash&body=Body"))
+      (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil)))
+      (should (string= (buffer-string) "[[file:///etc/mailcap][Triple Slash]]\nBody")))))
+
 (ert-deftest test-org-protocol/org-protocol-open-source ()
   "Test org-protocol://open-source links."
   (let* ((temp-file-name1 (make-temp-file "org-protocol-test1"))
-- 
2.25.1

Re: [PATCH 3/3] test-org-protocol.el: Add expected failures for file:/// URLs

[Bastien]

Applied as 3dbeb677e in master, thanks.

Maxim Nikulin <manikulin@gmail.com> writes:

> * testing/lisp/test-org-protocol.el
> (test-org-protocol/org-protocol-store-link-file,
> test-org-protocol/org-protocol-capture-file): Add tests to document
> that existing calls to `org-protocol-sanitize-uri' could make passed
> URLs invalid by changing number of slashes after scheme.

-- 
 Bastien