testcover: setf-method and treatment of `defcustom'

testcover: setf-method and treatment of `defcustom'

[Stefan Merten]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

Hi!

I just found the `testcover' package in Emacs 23.1.1 (the source in
Emacs 24 changed only very slightly). I tried it with my tests for
rst.el and after a lot of investigation got it to work with my `ert'
based test suite.

I had to solve two problems. When I ran my ert tests some of them fail
with

	No setf-method known for testcover-after

I looked into this and found `defsetf' in `cl-macs'. `testcover-after'
is a function which has only some side effect. I think

  (defsetf testcover-after (idx val) (store)
    (list 'progn
	  (list 'testcover-after idx val)
	  (list 'setf val store)))

does the right thing. At least the error messages described above
vanish for me.

AFAICS the only thing which can go wrong with this setf-method seems
to be the double evaluation of `val' - although I don't know whether
this really is evaluated twice in this context. Too much macro...

I still had another error left, however.

	 Value of form marked with `1value' does vary: ...

It took me some time to discover that for some reason `testcover'
treats `defcustom' like `defconst'. This is of course lethal for a
test which changes a customizable variable temporarily - e.g. by `let'
- to test a certain functionality.

For now I defined

  (defun rst-defcustom-testcover ()
    "Remove all customized variables from `testcover-module-constants'.
  This seems to be a bug in `testcover': `defcustom' variables are
  considered constants.  Revert it with this function after each `defcustom'."
    (when (boundp 'testcover-module-constants)
      (setq testcover-module-constants
	    (delq nil
		  (mapcar
		   (lambda (sym)
		     (if (not (plist-member (symbol-plist sym) 'standard-value))
			 sym))
		   testcover-module-constants)))))

and put a call to this function after every `defcustom'. This helps
but IMHO is of selected ugliness.

I'd rather suggest this patch (against Emacs 24 sources):

  --- ../emacs/trunk/lisp/emacs-lisp/testcover.el	2012-04-20 19:50:27.000000000 +0200
  +++ /home/stefan/tmp/testcover.el	2012-09-10 08:58:01.000000000 +0200
  @@ -297,7 +297,7 @@
	  (push (cadr form) testcover-module-1value-functions))
	 (when (eq val 'maybe)
	  (push (cadr form) testcover-module-potentially-1value-functions)))
  -     ((memq fun '(defconst defcustom))
  +     ((eq fun 'defconst)
	 ;;Define this symbol as 1-valued
	 (push (cadr form) testcover-module-constants)
	 (testcover-reinstrument-list (cddr form)))

In addition it would be certainly helpful to integrate the `defsetf'
form above somehow in `testcover'. However, I have no idea how to do
this in a good way.


						Grüße

						Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 307 bytes --]

Re: testcover: setf-method and treatment of `defcustom'

[Stefan Monnier]

> 	No setf-method known for testcover-after

Does one of the 2 patches work?

> I looked into this and found `defsetf' in `cl-macs'. `testcover-after'
> is a function which has only some side effect.  I think
>   (defsetf testcover-after (idx val) (store)
>     (list 'progn
> 	  (list 'testcover-after idx val)
> 	  (list 'setf val store)))
> does the right thing.  At least the error messages described above
> vanish for me.

I'm not sure what we should do, but the above doesn't sound quite right.

> AFAICS the only thing which can go wrong with this setf-method seems
> to be the double evaluation of `val'

In (setf (testcover-after IDX (aref A I)) V), your defsetf will be
called with a `val' that's a temporary variable bound to the value of
(aref A I), so double evaluation is not a problem.  But your `(setf ,val
,store) will end up changing some temporary variable rather than
changing the I'th slot of the A vector.

My 1st patch has the downside that it doesn't call testcover-after at all.
The reason is that I don't know what VAL to set in IDX when we do
things like (push VAL (testcover-after IDX PLACE)): should it be the
value read before pushing VAL onto it, or the value set afterwards?
The second patch chooses to call testcover-after when setting the value.

> I still had another error left, however.
> 	 Value of form marked with `1value' does vary: ...

I'll let Jonathan comment on this (as well as on my suggested gv-expanders).


        Stefan


=== modified file 'lisp/emacs-lisp/testcover.el'
--- lisp/emacs-lisp/testcover.el	2012-01-19 07:21:25 +0000
+++ lisp/emacs-lisp/testcover.el	2012-09-11 03:09:45 +0000
@@ -447,6 +447,7 @@
 (defun testcover-after (idx val)
   "Internal function for coverage testing.  Returns VAL after installing it in
 `testcover-vector' at offset IDX."
+  (declare (gv-expander (lambda (do) (gv-get val do))))
   (cond
    ((eq (aref testcover-vector idx) 'unknown)
     (aset testcover-vector idx val))



=== modified file 'lisp/emacs-lisp/testcover.el'
--- lisp/emacs-lisp/testcover.el	2012-01-19 07:21:25 +0000
+++ lisp/emacs-lisp/testcover.el	2012-09-11 03:38:33 +0000
@@ -447,6 +447,13 @@
 (defun testcover-after (idx val)
   "Internal function for coverage testing.  Returns VAL after installing it in
 `testcover-vector' at offset IDX."
+  (declare (gv-expander (lambda (do)
+                          (gv-letplace (getter setter) val
+                            (funcall do getter
+                                     (lambda (store)
+                                       (macroexp-let2 nil s store
+                                         `(progn (testcover-after ,idx ,s)
+                                                 ,(funcall setter s)))))))))
   (cond
    ((eq (aref testcover-vector idx) 'unknown)
     (aset testcover-vector idx val))

Re: testcover: setf-method and treatment of `defcustom'

[Stefan Merten]

[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]

Hi Stefan and Jonathan!

Yesterday Stefan Monnier wrote:
>> 	No setf-method known for testcover-after
> 
> Does one of the 2 patches work?

I'd need to compile and install an Emacs 24 version for this to try.
Frankly so far I didn't do so. I'll give it a try.

>> I looked into this and found `defsetf' in `cl-macs'. `testcover-after'
>> is a function which has only some side effect.  I think
>>   (defsetf testcover-after (idx val) (store)
>>     (list 'progn
>> 	  (list 'testcover-after idx val)
>> 	  (list 'setf val store)))
>> does the right thing.  At least the error messages described above
>> vanish for me.
> 
> I'm not sure what we should do, but the above doesn't sound quite right.

This may well be. I do not fully understand what I'm doing there ;-) .

>> AFAICS the only thing which can go wrong with this setf-method seems
>> to be the double evaluation of `val'
> 
> In (setf (testcover-after IDX (aref A I)) V), your defsetf will be
> called with a `val' that's a temporary variable bound to the value of
> (aref A I), so double evaluation is not a problem.

Ok.

> But your `(setf ,val
> ,store) will end up changing some temporary variable rather than
> changing the I'th slot of the A vector.

Oops. My intention with ",val" was to expand the original form before
binding it to val into this place so `setf' treats the original form.
May be that's not possible with `defsetf'.

> My 1st patch has the downside that it doesn't call testcover-after at all.
> The reason is that I don't know what VAL to set in IDX when we do
> things like (push VAL (testcover-after IDX PLACE)): should it be the
> value read before pushing VAL onto it, or the value set afterwards?

My cent: Before pushing VAL. AFAICS `testcover-after' needs to see
PLACE unmodified.


						Grüße

						Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 307 bytes --]

Re: testcover: setf-method and treatment of `defcustom'

[Stefan Monnier]

>> Does one of the 2 patches work?
> I'd need to compile and install an Emacs 24 version for this to try.

Actually, not just "Emacs 24" but "Emacs trunk" because it uses gv.el
which is new on the trunk.  For older Emacsen, I'd have to use
define-setf-expander which is a pain the rear.

>> But your `(setf ,val ,store) will end up changing some temporary
>> variable rather than changing the I'th slot of the A vector.
> Oops. My intention with ",val" was to expand the original form before
> binding it to val into this place so `setf' treats the original form.
> May be that's not possible with `defsetf'.

Indeed, that's not possible with `defsetf' which is macro designed
specifically for setters that do not care about the shape of their
argument, but only about the value they return.

>> My 1st patch has the downside that it doesn't call testcover-after at all.
>> The reason is that I don't know what VAL to set in IDX when we do
>> things like (push VAL (testcover-after IDX PLACE)): should it be the
>> value read before pushing VAL onto it, or the value set afterwards?
> My cent: Before pushing VAL. AFAICS `testcover-after' needs to see
> PLACE unmodified.

So for (setf (testcover-after IDX PLACE) VAL), we should first read the
value of PLACE, pass it to testcover-after, and then modify PLACE to
have value VAL?


        Stefan

Re: testcover: setf-method and treatment of `defcustom'

[Stefan Merten]

Hi Stefan!

Today Stefan Monnier wrote:
>>> Does one of the 2 patches work?
>> I'd need to compile and install an Emacs 24 version for this to try.
> 
> Actually, not just "Emacs 24" but "Emacs trunk" because it uses gv.el
> which is new on the trunk.

Sure.

>>> My 1st patch has the downside that it doesn't call testcover-after at all.
>>> The reason is that I don't know what VAL to set in IDX when we do
>>> things like (push VAL (testcover-after IDX PLACE)): should it be the
>>> value read before pushing VAL onto it, or the value set afterwards?
>> My cent: Before pushing VAL. AFAICS `testcover-after' needs to see
>> PLACE unmodified.
> 
> So for (setf (testcover-after IDX PLACE) VAL), we should first read the
> value of PLACE, pass it to testcover-after, and then modify PLACE to
> have value VAL?

This is my understanding.


						Grüße

						Stefan

Re: testcover: setf-method and treatment of `defcustom'

[Stefan Merten]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

Hi StefanMo!

5 days ago Stefan Monnier wrote:
>> 	No setf-method known for testcover-after
> 
> Does one of the 2 patches work?

I tried the second patch with the trunk Emacs. It works for me.

> My 1st patch has the downside that it doesn't call testcover-after at all.

This is certainly not what to do.

> The reason is that I don't know what VAL to set in IDX when we do
> things like (push VAL (testcover-after IDX PLACE)): should it be the
> value read before pushing VAL onto it, or the value set afterwards?
> The second patch chooses to call testcover-after when setting the value.

I'm not sure whether this reflects our latest discussion. If not I'm
now ready to test another patch.


						Grüße

						Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 307 bytes --]

Re: testcover: setf-method and treatment of `defcustom'

[Stefan Monnier]

> My cent: Before pushing VAL. AFAICS `testcover-after' needs to see
> PLACE unmodified.

Then please try the patch below instead.


        Stefan


=== modified file 'lisp/emacs-lisp/testcover.el'
--- lisp/emacs-lisp/testcover.el	2012-01-19 07:21:25 +0000
+++ lisp/emacs-lisp/testcover.el	2012-09-11 03:38:33 +0000
@@ -447,6 +447,13 @@
 (defun testcover-after (idx val)
   "Internal function for coverage testing.  Returns VAL after installing it in
 `testcover-vector' at offset IDX."
+  (declare (gv-expander (lambda (do)
+                          (gv-letplace (getter setter) val
+                            (funcall do getter
+                                     (lambda (store)
+                                       (macroexp-let2 nil s store
+                                         `(progn (testcover-after ,idx ,getter)
+                                                 ,(funcall setter s)))))))))
   (cond
    ((eq (aref testcover-vector idx) 'unknown)
     (aset testcover-vector idx val))

Re: testcover: setf-method and treatment of `defcustom'

[Stefan Merten]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

Hi!

2 days ago Stefan Merten wrote:
> I still had another error left, however.
> 
> 	 Value of form marked with `1value' does vary: ...
> 
> It took me some time to discover that for some reason `testcover'
> treats `defcustom' like `defconst'. This is of course lethal for a
> test which changes a customizable variable temporarily - e.g. by `let'
> - to test a certain functionality.

I now used `testcover' intensively and can see why there is a wish to
treat a `defcustom' like a `defconst'. If you treat a `defcustom' like
a normal `defvar' you get a brown splotch (i.e. the form had only a
single value) for all the `defcustom' variables. This is annoying.

OTOH in automated tests you *want* to override a `defcustom' to test
behavior with a different customization. So treating `defcustom' as
`defconst' unconditionally is certainly not an option.

The natural thing which comes to mind is to create a configuration
option here. For instance there could be another customizable
`testcover' variable which lists all the `defcustom' variables you
*do* want to treat as a variable.

How does this sound?


						Grüße

						Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 307 bytes --]