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

[Michael Albinus]

Hugh Daschbach <hugh@ccss.com> writes:

Hi Hugh,

> I could dig deeper into the dbus-introspect-get-interface, but wanted
> to come up for air first.  Let me know if you think it's worth the
> effort given the individual method, signal, and property tests.

Thanks for this!

It is already very comprehensive (and takes 2 seconds on my laptop, more
than all other tests together). So I guess we could take it as is, until
new tests are triggered by errors in the wild.

OTOH, I don't mind to give this test the :expensive-test tag. Then it
doesn't matter how long it runs.

Given the time it consumes, there might be a need to cache introspection
data. Either the result of dbus-introspect or dbus--parse-xml-buffer, I
guess rather the latter. Do you want to investigate it in dbus.el?

> And, of course, let me know what you think should be reworked.

Here we are. I don't repeat general comments I have given the other review.

> +(defun dbus--test-introspect ()
> +  "Return test introspection string."
> +  "<?xml version=\"1.0\"?>

...

Well, this is one approach. Alternatively, we could regard the
introspection file as test data, which is located in a file called
.../test/lisp/net/dbus-tests/org.gnu.Emacs.TestDBus.xml. This function
(the handler for the Introspect method) would read the file, and return
its contents.

> +(defsubst dbus--test-examine-interface (iface-name
> +                                        expected-properties
> +                                        expected-methods
> +                                        expected-signals
> +                                        expected-annotations)

This is rather C-style of argument indentation. In ELisp, we do
something like

(defsubst dbus--test-examine-interface
  (iface-name expected-properties expected-methods
   expected-signals expected-annotations)
...)

> +  (let ((interface (dbus-introspect-get-interface
> +                    :session
> +                    dbus--test-service
> +                    dbus--test-path
> +                    iface-name)))

A similar comment applies.

> +    (should-not (equal expected nil))

This is (should expected)

> +  (unwind-protect
> +      ;; dbus-introspect-get-node-names
> +      (should
> +       (equal
> +        (dbus-introspect-get-node-names :session dbus--test-service 
> dbus--test-path)
> +        '("node0" "node1")))

A (progn ... is missing after unwind-protect.

> +       '("org.freedesktop.DBus.Deprecated")))

Hmm. Maybe we shall give "org.freedesktop.DBus.Deprecated" a defconst in
dbus.el?

> Thanks,
> Hugh

Best regards, Michael.