GNU bug report logs - #75633
[PATCH] New unit-tests for cl-lib

Previous Next

Full log

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

From: "Basil L. Contovounesios" <basil <at> contovou.net>
To: Ahmed Khanzada <me <at> enzu.ru>
Cc: eliz <at> gnu.org, 75633 <at> debbugs.gnu.org
Subject: Re: bug#75633: [PATCH] New unit-tests for cl-lib
Date: Sat, 25 Jan 2025 19:52:59 +0100

[Message part 1 (text/plain, inline)]

Ahmed Khanzada [2025-01-17 09:52 -0500] wrote:

> Subject: [PATCH] New unit-tests for cl-lib

Thank you for improving the test suite!
I include some comments, nits, and a patch below.

Ahmed Khanzada [2025-01-18 09:19 -0500] wrote:

> Yes, I will look into the cl-seq-tests byecompiling warnings and fix them.

The warnings drew my attention to several preexisting bugs in
cl-seq-tests.el, with us since bug#24264.  For example, none of the
tests using cl-seq--with-side-effects were actually being evaluated.

I thus took the liberty of scrutinising the file a bit closer and attach
the resulting patch.

I would like to apply it if there are no objections; otherwise feel free
to use/adapt it as desired.

> +  (should (equal '(2) (cl-remove-if-not 'evenp '(2) :key #'(lambda (x) (- x))))))
                                                           ^^^^^^^^^^^^^^^^^^^^
AKA #'-.
Also, there is no need to #'-quote lambdas, as they are self-quoting.

> +(ert-deftest cl-delete-if-test ()
> +  (let ((list (list 1 2 3 4 5)))

Only used once, and unlike the remaining test cases which use no
variable.

> +    (cl-delete-if 'evenp list)

It's more robust to check the return value of deletion functions, rather
than rely on their side effects.  This would also pacify the
byte-compiler warning.

> +(ert-deftest cl-delete-if-not-test ()
> +  (let ((list (list 1 2 3 4 5)))

Ditto re: gratuitous binding.

> +  (let ((result (cl-substitute-if 'x #'(lambda (n) t) '(1 2 3 4 5))))
                                       ^^^^^^^^^^^^^^^^
AKA (lambda (_) t)
or  #'always.
This would also pacify the byte-compiler.

> +  (let ((result (cl-count-if (lambda (x) nil) '(1 2 3 4))))
                               ^^^^^^^^^^^^^^^^
AKA (lambda (_) nil)
or  (lambda (_))
or  #'ignore.
This would also pacify the byte-compiler.

> +(ert-deftest cl-sort-test ()
> +  (let ((result (cl-sort '(3 1 4 1 5 9 2 6 5 3 5) '<)))

cl-sort and cl-stable-sort are documented as being destructive
functions, so it's unsafe to pass them an 'immutable' quoted literal;
for details see (info "(elisp) Mutability").

Passing a fresh list would also pacify the byte-compiler.

> +  (let ((result (cl-sort '("banana" "fig" "apple" "kiwi") (lambda (x y) (< (length x) (length y))) :key 'identity)))

This and other lines are slightly long.

> +  (let ((result (cl-sort (vector 3 1 4 1 5) '<)))
> +    (should (equal result (vector 1 1 3 4 5))))

While the argument to cl-sort ought to be fresh/mutable, its result can
safely be compared to a quoted value such as [1 1 3 4 5].

> +(ert-deftest cl-merge-test ()
> +  (let ((result (cl-merge 'list '(1 3 5) '(2 4 6) '<)))

Ditto re: passing a fresh value to cl-merge, which is destructive.

> +  (let ((result (cl-member-if #'(lambda (x) (< x 0)) '(1 2 3 4 5))))
                                ^^^^^^^^^^^^^^^^^^^^^^
AKA #'cl-minusp.

> +  (let ((result (cl-assoc-if-not #'(lambda (x) (> x 0)) '((1 . "one") (2 . "two") (3 . "three")))))
                                   ^^^^^^^^^^^^^^^^^^^^^^
AKA #'cl-plusp.

> +  (let ((result (cl-intersection '(1 "two" 3) '(3 "two" 4))))
> +    (should (equal result '(3))))

This assumes (not (eql "two" "two")), which I'm not sure can always be
guaranteed in the face of optimisations.  I would replace at least one
of the string literals with a fresh one.

> +(ert-deftest cl-nintersection-test ()
> +  (let ((list1 '(1 2 3 4))
> +        (list2 '(3 4 5 6)))
> +    (let ((result (cl-nintersection list1 list2)))

Ditto re: passing fresh values to cl-nintersection, which is a
destructive function.

> +      (should (equal list1 '(1 2 3 4)))
> +      (should (equal list2 '(3 4 5 6)))))

This seems dubious: list1 and list2 are quoted literals which may have
been modified by side effect.

Besides, the docstring of cl-nintersection (and other destructive
functions) explicitly claims to modify its arguments, so I don't
understand why we would want to check that they remain unmodified.

> +  (let ((list1 '(1 "two" 3))
> +        (list2 '(3 "two" 4)))
> +    (let ((result (cl-nintersection list1 list2)))

Ditto re: assuming (not (eql "two" "two")) always holds.

> +  (let ((result (cl-set-difference '((1 . "one") (2 . "two") (3 . "three"))
> +                                    '((1 . "uno") (2 . "dos"))
> +                                    :key 'car)))

Indentation.

> +  (let ((list1 '(1 2 3))
> +        (list2 '(2 3 4)))
> +    (cl-set-difference list1 list2)
> +    (should (equal list1 '(1 2 3)))
> +    (should (equal list2 '(2 3 4)))))

This assumes that modifying the literal '(1 2 3) in the list1 argument's
value will not also modify the same literal in the subsequent
(equal list1 '(1 2 3)), which I'm not sure is robust in the face of
optimisations (and similarly for list2).

To detect (the absence of) modification, I would use fresh arguments.

It also doesn't hurt to check the return value of cl-set-difference,
which would pacify the byte-compiler.

> +(ert-deftest cl-nset-difference-test ()
> +  (let ((list1 '(1 2 3 4))
> +        (list2 '(3 4 5 6)))
> +    (let ((result (cl-nset-difference list1 list2)))

Ditto re: passing fresh values to cl-nset-difference, which is a
destructive function.

> +  (let ((result (cl-set-exclusive-or '(1 2 3 4 5) '(3 4 5 6 7)))
> +        (list1 '(1 2 3 4 5))
> +        (list2 '(3 4 5 6 7)))
> +    (should (equal result '(1 2 6 7)))
> +    (should (equal list1 '(1 2 3 4 5)))
> +    (should (equal list2 '(3 4 5 6 7)))))

Ditto re: detecting modifications to quoted literals.

More importantly, list1 and list2 are not actually used.

> +(ert-deftest cl-nset-exclusive-or-test ()
> +  (let ((list1 '(1 2 3))
> +        (list2 '(3 4 5)))
> +    (let ((result (cl-nset-exclusive-or list1 list2)))

Ditto re: passing fresh values to cl-nset-exclusive-or, which is a
destructive function.

> +(ert-deftest cl-subsetp-test ()
> +  (let ((result (cl-subsetp '(1 2) '(1 2 3 4))))
> +    (should (equal result t)))

The docstring of cl-subsetp mentions a 'true' return value, and its
entry in the manual says nothing about the return value.

While 'true' generally means t, in the absence of a stronger indication
I would just accept any non-nil return value here.

> +(ert-deftest cl-lib-set-difference ()
> +(ert-deftest cl-nset-difference ()

Why are there tests for cl-set-difference and cl-nset-difference in both
cl-lib-tests.el and cl-seq-tests.el?

Thanks,
-- 
Basil

[0001-Fix-cl-seq-tests.el.patch (text/x-diff, attachment)]

Previous Next

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