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

[Hugh Daschbach]

Michael Albinus writes:

> Hugh Daschbach <hugh@ccss.com> writes:
>
> Hi Hugh,
>
> Thanks for this. AFAICS, there's nothing left open for 
> bug#43252; I'd
> like to close it. Is this OK for you?

Yes.  I was about to suggest this.  The issue is resoled.

> > And, of course, let me know what you think should be reworked.
>
> There are only some nits left to comment.

Good.  Nits promote quality and consistency.

> > 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.

Good.  Fixed.

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

I've taken a shot at this.  I'm not sure I got the ChangeLog style
correct.  Let me know if I'm still off the beaten path.

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

Fixed.

> > +(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 :-)

You say that as if were a bad thing :-).

Fixed.

> > +        (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?

I seem to consistently stumble over compound type syntax.  Thanks 
for
point this out.  Both forms are now tested.

> > +        (should-error                   ; Cannot set property 
> > with :read access
> > +         (equal ...))
>
> 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.

Got it.  I think I've fixed each should-error test.

> > +        ;; Test integer overflow
>
> I don't see any integer *overflow* in the following tests.

Yes, really botched that.  I had a byte truncation test that I 
only now
realize isn't treated as an integer.  And that wasn't near the 
comment.
Sigh.

I've added conforming and overflow tests.  The overflow test,
appropriately, error on register.

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

Done.

> 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?

Sure.  I'll have a look.  If I find something useful, I'll open 
another
bug.

> > 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.

Makes sense.  Done.

> > +(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

Guilty as charged.  I'm still working on developing idiomatic 
elisp.

> (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.

Is:

(let* ((property
         (dbus-introspect-get-property
          :session
          dbus--test-service
          dbus--test-path interface
          property-name))) ...)

preferred over:

(let* ((property
         (dbus-introspect-get-property :session 
         dbus--test-service
          dbus--test-path interface property-name)))
   ...)

If not, I'll take another bite at the apple.

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

Yes, I should read what I write.  Fixed.  Thanks.

> > +  (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.

I really should be more careful when I rip out instrumentation. 
Thanks
for catching this.

> > +       '("org.freedesktop.DBus.Deprecated")))
>
> Hmm. Maybe we shall give "org.freedesktop.DBus.Deprecated" a 
> defconst in
> dbus.el?

Done.  ’dbus-annotation-deprecated’.  Let me know if you think 
this should be
‘dbus--annotation-deprecated’ instead.

> > Thanks,
> > Hugh
>
> Best regards, Michael.

In other news, this test:

 (should-error
  (dbus-check-arguments :session dbus--test-service :unix-fd 10)
  :type 'dbus-error)

works for me in batch mode, but not interactive mode.

On GNU/Linux, (dired (format "/proc/%s/fd" (emacs-pid))) indicates 
I
have 14 open file descriptors on a test instance (emacs -q -Q). 
On my
development instance, I've got 27 open file descriptors.

Cheers,
Hugh