[PATCH consfigurator v3 3/8] run tests with temporary gpg home

[Sean Whitton]

Hello,

On Sun 13 Mar 2022 at 11:40am -03, David Bremner wrote:

> diff --git a/tests/runner.lisp b/tests/runner.lisp
> new file mode 100644
> index 0000000..0a9867b
> --- /dev/null
> +++ b/tests/runner.lisp
> +
> +(defun first-gpg-fingerprint ()
> +  "Return the fingerprint of the first (primary) key listed by gpg.  This is
> +mainly useful when there is a single primary key."

Could you add two newlines after the first sentence, like an Emacs
docstring, please -- I'll add something to CONTRIBUTING.rst about this.

> +
> +(defun with-test-gnupg-home-func (base-dir thunk)
> +  "Implementation for macro WITH-TEST-GNUPG-HOME."
> +  (let ((*data-source-gnupghome*
> +          (ensure-directory-pathname #?"${base-dir}/gnupg")))
> +    (unless (nth-value 1
> +             (ensure-directories-exist *data-source-gnupghome* :mode #o700))
> +      (error "~s already exists" *data-source-gnupghome*))
> +    ;; Create a passwordless key using gnupg's defaults.
> +    (gpg '("--batch" "--pinentry-mode" "loopback" "--passphrase" "" "--yes"
> +           "--quick-generate-key" "consfig at example.org (insecure!)"))

Oh nice, this seems better that storing a key, though perhaps later
we'll need to find a way to turn down the quality of the random bytes.

> +    (let ((*test-gnupg-fingerprint* (first-gpg-fingerprint)))
> +      (with-open-file (stream #?"${*data-source-gnupghome*}/gpg.conf"
> +                              :direction :output)
> +        (format stream "default-key ~a~%default-recipient-self~%"
> +                *test-gnupg-fingerprint*))
> +      (funcall thunk))
> +    (run-program "gpgconf" "--homedir" *data-source-gnupghome*
> +                 "--kill" "all")))

I think you want an UNWIND-PROTECT to ensure that the process gets
killed even if the test code exits abnormally.  However:

> +(defmacro with-test-gnupg-home (base-dir &rest body)
> +  "Set up gnupg homedir for test suite under BASE-DIR and run BODY with
> +*DATA-SOURCE-GNUPGHOME* set appropriately."
> +  `(with-test-gnupg-home-func ,base-dir (lambda () (progn , at body))))

This macrology is somewhat unidiomatic.  My thought is that just looking
at the defmacro doesn't make it clear that the primary reason there's a
macro is to establish a dynamic binding for *DATA-SOURCE-GNUPGHOME*.
Instead, it looks like you're going to do something fancy with closures.

I think a more idiomatic style would be like this:

    `(let ((*data-source-gnupghome* ...))
       (with-test-gnupg-home-func ,base-dir) ; probably rename it
       (unwind-protect (progn , at body)
         (run-program "gpgconf" ...)))

> +;; tests for test runner machinery

Could you use

    ^L
    ;;;; Test for test runner machinery

    (deftest ..

-- following the idea that two semicolons is for comments applying only
to the immediately proceeding code.  I'll put that in CONTRIBUTING.rst.

> +
> +(defun runner ()
> +  "Run tests via (sb-)rt, with setup and teardown"

There's a period missing from the end of the docstring here.

-- 
Sean Whitton