GNU bug report logs - #72574
[PATCH 0/3] debbugs improvements. Add tests

Previous Next

Full log

Message #20 received at 72574 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: 72574 <at> debbugs.gnu.org
Subject: Re: [PATCH 3/3] Add tests
Date: Sun, 11 Aug 2024 15:53:20 +0200

Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:

Hi Morgan,

here are my comments:

> * test/test-debbugs.el: New file.

I propose to follow Emacs naming conventions. A file PACKAGE.el has a
corresponding file PACKAGE-tests.el. So we might call the file now
debbugs-tests.el. If more test functions arrive, we might split this
file into the different debbugs-*-tests.el files.

> +++ b/Makefile
> +.PHONY: check
> +
> +check:
> +	emacs -Q --batch -L . -l test/* -f ert-run-tests-batch-and-exit

Well, you run the tests on the source file. Usually, batch tests are
run on compiled files. So you might add rules for compiling the
debbugs*.el, and the test/debbugs-tests*.el files first.

You call the program 'emacs', hardcoded. This is OK as default, but
people (me!) might want to test with different Emacs versions on the
same machine. Please add a variable 'EMACS ?= emacs', and use it.

Don't use '-l test/*', better is -l 'test/*.el'. Perhaps you want to add
something else in that directory, like a README?

> +++ b/test/test-debbugs.el
> +;;; test-debbugs.el --- tests for debbugs.el -*- lexical-binding: t; -*-

The Copyright notice and the GNU GPL text is missing. And also the
Author: and Package: header lines.

> +;; Generated using this:
> +;; (soap-invoke debbugs-wsdl debbugs-port "get_status" [64064])
> +(defvar test-bug-status-soap-return

Please prefix helper objects with 'debbugs-test--'. Yes, two hyphens,
'cos they are helpers.

It has a fixed value, so it might be a defconst.

> +;; Generated using this:
> +;; (debbugs-get-status 64064)
> +(defvar test-bug-status

Same comments as above.

> +(defvar test-soap-operation-name nil)
> +(defvar test-soap-parameters nil)

Please adapt the names.

> +(defun soap-invoke-internal (callback _cbargs _wsdl _service operation-name &rest parameters)
> +  "Over-ride for testing"
> +  (setq test-soap-operation-name operation-name)
> +  (setq test-soap-parameters parameters)
> +  (let ((return
> +         (cond ((string-equal operation-name "get_status") test-bug-status-soap-return)
> +               ((string-equal operation-name "get_usertag") '(((hi))))
> +               (t '((0))))))
> +    (if callback
> +        (progn
> +          (funcall callback return)
> +          nil)
> +      return)))

Don't override the function this way. It is better to write a helper
function debbugs-test--soap-invoke-internal, and to override the
original function with something like

--8<---------------cut here---------------start------------->8---
(add-function
 :override (symbol-function #'soap-invoke-internal)
 #'debbugs-test--soap-invoke-internal)
--8<---------------cut here---------------end--------------->8---

Another technique is to use cl-letf, see example below.

> +(ert-deftest test-debbugs-get-bugs ()

Please rename all tests to debbugs-test-*. This is not formalized in the
Emacs source tree, but good practice.

> +(ert-deftest test-debbugs-get-status ()
> +  (let ((original-float-time (symbol-function 'float-time))
> +        test-soap-operation-name test-soap-parameters)
> +    (fset 'float-time (lambda (&optional _specified-time) 5000))
> +    (should (= (float-time) 5000))
> +    (should (equal (sort (car (debbugs-get-status 64064))) (sort (car test-bug-status))))
> +    (should (string-equal test-soap-operation-name "get_status"))
> +    (should (equal test-soap-parameters '([64064])))
> +    (fset 'float-time original-float-time)))

Please respect max line width of 80.

Please use cl-letf instead of fset. Something like

--8<---------------cut here---------------start------------->8---
(ert-deftest debbugs-test-get-status ()
  (cl-letf (((symbol-function #'float-time)
	     (lambda (&optional _specified-time) 5000)))
    (let (test-soap-operation-name test-soap-parameters)
      (should (= (float-time) 5000))
      (should (equal (sort (car (debbugs-get-status 64064)))
                     (sort (car test-bug-status))))
      (should (string-equal test-soap-operation-name "get_status"))
      (should (equal test-soap-parameters '([64064]))))))
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.