bug#43252: 27.1; DBus properties lack type hints or overrides

[Michael Albinus <michael.albinus@gmx.de>]

Hugh Daschbach <hugh@ccss.com> writes:

Hi Hugh,

In general, your tests are very useful. Thanks!

Just some comments on your patch.

> Add tests that should fail, like setting a property with a type
> different from it's type at registration time.

As said the other message, this constraint doesn't exist any longer.
Registered services might want to control, which properties are
set. This could be the type of a property, or a restriction of the value
(for example, just a predefined set of strings could be allowed).

Maybe, we give these services such a mean? That is, they could add an
own handler function in dbus-register-property, which is applied when a
org.freedesktop.DBus.Properties.Set method call is handled. WDYT?

> +(defun dbus-test06-make-property-test (selector name value expected)

I would call it dbus--test-make-property-test. test06 isn't important,
and the double slash "--" is an indication that this is an internal
function, which shouldn't leave the dbus-tests.el scope. See the other
helper functions in the file.

> +;; Since we don't expect this helper function and it's caller
> +;; `dbus-test06-make-property' to be used outside this file, we don't
> +;; bother with `eval-and-compile.'  It would be appropriate to wrap
> +;; this with `eval-and-compile' if that expectation is misguided.

Well, it is uncommon that a function returns a code snippet. I haven't
checked, but couldn't you achieve your goal by changing this defun into
a defsubst?

> +(defmacro dbus-test06-test-property (name value-list)

Same comment on name here. I would call it dbus--test-property.

> +The argument VALUES is a list of pairs, where each pair
> +represents a value form and an expected returned value form.  The
> +first pair in VALUES is used for `dbus-register-property'.
> +Subsequent pairs of the list are tested with
> +`dbus-set-property'."

The second argument is VALUE-LIST, not VALUES. However, Elisp encourages
an argument list like

(defmacro dbus-test-test-property (name &rest value-list)

This simplifies call conventions, you can call then with several
key-value arguments like

(dbus--test-property
 "ByteArray"
 '((:array :byte 1 :byte 2 :byte 3) . (1 2 3))
 '((:array :byte 4 :byte 5 :byte 6) . (4 5 6)))

> +(defmacro with-dbus-monitor (buffer &rest body)

Such a macro name would poison your Elisp name space. Keep the
dbus--test prefix, and name the macro like dbus--test-with-dbus-monitor.

> +    (unwind-protect
> +        (progn ,@body)
> +      (sit-for 0.5)

sit-for is problematic, because it would delay the test run by 0.5
seconds, unconditionally. People regard this negative, because the
(whole) Emacs test suite shall run fast. A better check might be

(with-timeout (1 (dbus--test-timeout-handler))
  (while (accept-process-output process 0 nil t)))

> +          (should
> +           (equal
> +            (dbus-register-property
> +             :session dbus--test-service dbus--test-path
> +             dbus--test-interface "StringArray" :read
> +             '(:array "one" "two" :string "three"))
> +            `((:property :session ,dbus--test-interface "StringArray")
> +	      (,dbus--test-service "/org/gnu/Emacs/TestDBus"))))

You might use ,dbus--test-path instead. Here and everywhere else.

> +
> +          (should                             ; Should this error instead?
> +           (equal
> +            (dbus-set-property
> +             :session dbus--test-service dbus--test-path
> +             dbus--test-interface "StringArray"
> +             '(:array "seven" "eight" :string "nine"))
> +            nil))

Good question. dbus-set-property and dbus-get-property do not propagate
D-Bus errors. Maybe we shall change the functions to do so? I've asked
this already myself.

> +        ;; Test mismatched types in array
> +
> +        (should                         ; Oddly enough, register works, but get fails
> +         (equal
> +          (dbus-register-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "MixedArray" :readwrite
> +           '(:array
> +             :object-path "/node00"
> +             :string "/node01"
> +             :object-path "/node0/node02"))
> +          `((:property :session ,dbus--test-interface "MixedArray")
> +	    (,dbus--test-service "/org/gnu/Emacs/TestDBus"))))

Hmm, yes. dbus-register-property does not perform a local type
check. And honestly, I don't want to do it; I let the D-Bus daemon do
the job.

> +        (should-error
> +         (equal
> +          (dbus-get-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "MixedArray")
> +          '("/node00" "/node01" "/node0/node02")))

Yes, dbus-get-property is hit by the mismatched types in the :array. Isn't
this sufficient?

> +        (should                             ; This should error or the next get should fail
> +         (equal
> +          (dbus-set-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "ByteValue" 1024)
> +          1024))

No error expected. You haven't given 1024 a type (like :byte), so it is
handled as :uint32.

> +        ;; Test invalid type specification
> +
> +        (should
> +         (equal
> +          (dbus-register-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "InvalidType" :readwrite
> +           :keyword 128)
> +          `((:property :session ,dbus--test-interface "InvalidType")
> +	    (,dbus--test-service "/org/gnu/Emacs/TestDBus"))))

Oops. This shall be detected in dbus-register-property.

> Cheers,
> Hugh

Best regards, Michael.