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

[Michael Albinus]

Hugh Daschbach <hugh@ccss.com> writes:

Hi Hugh,

> I have attached two patches for your review.  I think the property
> tests are complete; I've adjusted the tests to expect errors on
> register or set rather than get.

Thanks for this. AFAICS, there's nothing left open for bug#43252; I'd
like to close it. Is this OK for you?

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

There are only some nits left to comment.

> Add DBus tests to validate property handling.  Includes cycling

We call this beast D-Bus, with a hyphen. Here and everywhere in the
docstrings and comments.

> register, get, set, get, GetAll, and GetManagedObjects over
> several property types.
>
> Add tests that should fail, like setting a property with a type
> different from it's type at registration time.

We add also a ChangeLog style entry to the commit message, see the git
log of dbus-tests.el.

> +The argument SELECTOR indicates whether the test should expand to
> +'dbus-register-property' (if SELECTOR is 'register) or

`dbus-register' (if SELECTOR is `register') or

> +(ert-deftest dbus-test06-test-property-types ()

The "-test" part of the name seems to be superfluous; I'd call it
dbus-test06-property-types. (You see, just nitpicks :-)

> +        (dbus--test-property
> +         "Dictionary"
> +         '((:array
> +            :dict-entry (:string "four" (:variant :string "value of four"))
> +            :dict-entry ("five" (:variant :object-path "/nodex"))
> +            :dict-entry ("six"  (:variant (:array :byte 4 :byte 5 :byte 6))))

This is one possibility to declare a :dict-entry. The other possibility,
with the same result, is

'((:array
   (:dict-entry :string "four" (:variant :string "value of four"))
   (:dict-entry "five" (:variant :object-path "/nodex"))
   (:dict-entry "six"  (:variant (:array :byte 4 :byte 5 :byte 6))))
...

Do you mind to test both?

> +        (should-error                   ; Cannot set property with :read 
> access
> +         (equal
> +          (dbus-set-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "StringArray"
> +           '(:array "seven" "eight" :string "nine"))
> +          nil))

The error happens in dbus-set-property. No need to test further for
equal. So you could do

(should-error                   ; Cannot set property with :read access
 (dbus-set-property
  :session dbus--test-service dbus--test-path
  dbus--test-interface "StringArray"
  '(:array "seven" "eight" :string "nine")))

Furthermore, you could test which error is raised (should-error allows
this). Something like

(should-error                   ; Cannot set property with :read access
 (dbus-set-property
  :session dbus--test-service dbus--test-path
  dbus--test-interface "StringArray"
  '(:array "seven" "eight" :string "nine"))
 :type 'dbus-error)

Similar approach for your other should-error forms.

> +        ;; Test integer overflow

I don't see any integer *overflow* in the following tests.

> Thanks,
> Hugh

Best regards, Michael.