Hi Maxime, Thank you for your comments!
On Thu, Apr 1, 2021 at 4:37 AM Maxime Devos <maximede...@telenet.be> wrote: > For example, in: > > > (define (%test-comp2 comp x) > > (syntax-case (list x (list (syntax quote) (%test-source-line2 x)) > > comp) () > > (((mac tname expected expr) line comp) > > (syntax > > - (let* ((r (test-runner-get)) > > - (name tname)) > > + (let ((r (test-runner-get))) > > (test-result-alist! r (cons (cons 'test-name tname) line)) > > (%test-comp2body r comp expected expr)))) > > I would keep the let* (but reverse the binding order), but change 'tname' > with 'name' in the call to 'test-result-alist!', such that 'test-X' macros > behave somewhat more like procedure calls (except for installing exeption > handlers and having access to the s-expression of the code that will be run, > of course). It's largely a matter of taste, though. > I've done this change. One thing I don't understand is the "reverse the binding order", I've done it as suggested but is this change the one you refer to as "matter of taste"? > In any case, it is good that 'tname' is now evaluated only once, as per > SRFI-64 (notice ***It is evaluated only once.*** (markup mine)): > > (test-assert [test-name] expression) > > This evaluates the expression. The test passes if the result is true; > if the result is false, a test failure is reported. The test also fails > if an exception is raised, assuming the implementation has a way to catch > exceptions. How the failure is reported depends on the test runner > environment. > The test-name is a string that names the test case. (Though the test-name is > a string literal in the examples, it is an expression. ***It is evaluated > only once.***) > It is used when reporting errors, and also when skipping tests, as described > below. > It is an error to invoke test-assert if there is no current test runner. > > (My suggestion would be to also evaluate 'test-name' at least once, even if > there > is no test runner, which seems a bit stricter than SRFI-64 demands, but seems > like > a nice property to have and easy to achieve.) > Yes, this makes sense. Thanks again for pointing that out. > As this patch does not ‘merely’ fix a warnings, but fixes a bug, could you > change > the patch message accordingly? Something like > > srfi-64: fix double evaluation of test-name. > > perhaps? > Sounds good to me. Best, Aleix
