Unit test macro

I have written a couple of macros (? and ??) for performing unit tests, but I'm having some difficulty with modifying it, so I was hoping to get some feedback on how to tidy up what I have written so far.

Yes, I know that unit test frameworks already exist, but I'm trying to learn by doing.

Here is an example of usage:

(??
  (? "Arithmetic tests"
    (? "Addition"
        (= (+ 1 2) 3)
        (= (+ 1 2 3) 6)
        (= (+ -1 -3) -4))))

And an example of output:

[Arithmetic tests]
  [Addition]
    (PASS) '(= (+ 1 2) 3)'
    (PASS) '(= (+ 1 2 3) 6)'
    (PASS) '(= (+ -1 -3) -4)'

Results: 3 tests passed, 0 tests failed

Now, the existing code works. Unfortunately, the (? ...) macro implementation is ugly, verbose, resistant to change - and I'm pretty sure also badly structured. For example, do I really have to use a list to store pieces of output code and then emit the contents at the end?

I'd like to modify the macro to permit description strings (or symbols) to optionally follow each test, whereupon it would replace the test literal in the output, thus:

(??
  (? "Arithmetic tests"
    (? "Addition"
        (= (+ 1 2) 3)    "Adding 1 and 2 results in 3"
        (= (+ 1 2 3) 6)
        (= (+ -1 -3) -4))))

Output:

[Arithmetic tests]
  [Addition]
    (PASS) Adding 1 and 2 results in 3
    (PASS) '(= (+ 1 2 3) 6)'
    (PASS) '(= (+ -1 -3) -4)'

But unfortunately I can't find a sensible place in the macro to insert this change. Depending on where I put it, I get errors like you're not inside a backquote expression, label is not defined or body-forms is not defined. I know what these errors mean, but I can't find a way to avoid them.

Also, I'll be wanting to handle exceptions thrown during the test, and treat that as a failure. Currently, there is no exception handling code - the test result is merely tested against nil. Again, it is not clear how I should add this functionality.

I'm thinking that maybe this macro is over

I can't quite figure out which mode you're using for indentation,
suffice to say that standard lisp-mode indents this a bit differently,
but never mind that. The single-line parenthesis really shouldn't be
there though, makes it look more ugly than necessary.

It's nice that the code is self-sufficient, though just in case that
that's not clear: There are general-purpose helper libraries (for
something like with-gensyms etc.) as well as for ANSI escape
formatting available.

Btw. while the colour formatting might be nice, there should be a switch
to turn it off and it would be even nicer if it could detect when it
wasn't running in the shell, so that using e.g. SLIME i wouldn't have to
see garbage on the screen.

In general, macro arguments shouldn't be evaluated multiple times! That
is the case for all macros where you currently use one of the arguments
more than once with , in the produced body. Reworking that to bind to
a temporary variable first also ensures that passing in a literal will
work. If you intent to modify a place, clearly indicate that in the
documentation, the macro argument names and still only evaluate the
initial value once. Otherwise you'll either end up having bugs or less
then ideal performance due to repeated accessing of computation-heavy
accessors and such.

Now I've also noticed that with-gensyms is used inside the produced
body. That's almost certainly wrong. Consider the expansion of
pop-first-char:

(let ((x "foo")) (pop-first-char x)))

=>

(LET ((X "foo"))
  (LET ((C (GENSYM)))
    (IF (> (LENGTH X) 0)
        (PROGN
         (SETQ C (SCHAR X 0))
         (IF (> (LENGTH X) 1)
             (SETQ X (SUBSEQ X 1))
             (SETQ X ""))))
    C))

gensym is called every time this form is evaluated, bound to c and
then c is still assigned later and returned. It's not harmful in this
case, because there's no forms passed into the macro that are spliced in
below it. I'll explain below how this should be rewritten instead.

So, going definition by definition:

• starts-with-p is inefficient as you can just compare from the start

and abort early if there's no match. The comparison can also be
(eql 0 p) which will work in all circumstances.

-
pop-first-char is, as explained, not quite so well written.
with-gensyms isn't needed and it should fail if there's nothing in
the string in the first place instead of returning a gensym symbol.
Also, (if ... (progn)) is better written as (when ... ...) or
using cond and schar should probably be char, aref or even
elt as this pattern works for all sequences, not just strings.
Consider this instead:

(defmacro pop-first-char (place)
  (with-gensyms (s)
    `(let ((,s ,place))
       (prog1 (schar ,s 0)
         (setf ,place (subseq ,s 1))))))

This will at least behave more consistently with respect to empty
strings (error) and evaluates place only as often as necessary.

• pop-chars can also be written much more efficiently. The

combination of pop-first-char, which will copy the rest of the
string and the loop introduces unnecessary quadratic behaviour; it
would be much better, say, to subseq the range you want, then
coerce that to a list and assign the rest.

• format-ansi-codes looks okayish, but note that the operations aren't

the most efficient (nreverse on the result, allocating in the right
order is likely allocating less memory).

• kv-lookup uses eql on strings - that's not working, compare the

spec.
You want equal or string-equal on the strings. Then again, I'd
consider this behaviour an unnecessary feature.

• format-ansi-escaped-arg could use typecase.

• In while-pop the seq isn't used (which SBCL tells you when loading

the file. The progn isn't necessary and can be simplified away.
IMO do is notoriously hard to understand, so I'd recommend not using
it. The parameters are badly named as pop only works on lists.
sequence (which should be list) should also be assigned to a
temporary variable.

I'll stop here; you get the gist of it by following the first examples.

Now for your question, I dislike the optional docstring feature because
it makes implementation more complicated since you'll always be checking
if there's a literal string coming afterwards. Having a straightforward
way to mark individual tests with a macro is completely obvious to the
reader and the developer, so I'd rather go with that.

In case you really wanted to go ahead, either consider a pattern
matching library, or don't do the whole while-pop thing and loop over
the list with one token lookahead, i.e.
(next-form possibly-a-string-here ...).

The other general recommendation is to decompose your functionality into
functions. Then for one testing becomes a bit easier as you can just
invoke the functions normally instead of having to macroexpand or
whatever your editor does, but also it allows you to demarcate the
different steps clearly by nami

StackExchange Code Review Q#102548, answer score: 3