[PATCH] Test-suite update

[PATCH] Test-suite update

[Ludovic Courtès]

[Switched to `guile-devel'.]

Kevin Ryde <user42@zip.com.au> writes:

> You'll need to make something for
> test-suite/tests/socket.test exercising each case, eventually.

It turns out that I had never looked at the test-suite, mostly because
it isn't run at `make check' time.  Additionally, the `guile-test'
script was largely outdated.

The patch below fixes these two things.  It also comments out the
`elisp' test which results in a stack overflow here.  My understanding
is that the Elisp work is almost only due to Neil has never been widely
used, so hopefully it won't hurt too much until a better solution is
found.

All the other tests pass on my PPC GNU/Linux box, some of them are
unresolved though.

Thanks,
Ludovic.


2005-09-22  Ludovic Courtès  <ludovic.courtes@laas.fr>

	* Makefile.am (check-local):  New target.
    
	* guile-test:  Made executable as a stand-alone script.
	(main):  Fixed to use the latest `getopt-long';  use `option-ref'.
    
	* tests/elisp.test:  Momentarily ignored.

\f
--- orig/test-suite/Makefile.am
+++ mod/test-suite/Makefile.am
@@ -101,3 +101,6 @@
 	  cp -pR $(srcdir)/$$d $(distdir)/$$d; \
           rm -rf $(distdir)/$$d/CVS; \
         done
+
+check-local:
+	./guile-test --test-suite tests/


--- orig/test-suite/guile-test
+++ mod/test-suite/guile-test
@@ -1,5 +1,12 @@
-#!../libguile/guile \
--e main -s
+#!/bin/sh
+# This is in fact -*- Scheme -*- code.
+
+# We need to make sure that we load the right `ice-9' modules so `-L' is not
+# enough since it gets parsed too late.
+GUILE_LOAD_PATH=".."
+export GUILE_LOAD_PATH
+
+exec ../libguile/guile -L .. -l $0 -e main -- "$@"
 !#
 
 ;;;; guile-test --- run the Guile test suite
@@ -166,7 +173,7 @@
     (sort tests string<?)))
 
 (define (main args)
-  (let ((options (getopt-long args
+  (let ((options (getopt-long (cons "progname" args)
 			      `((test-suite
 				 (single-char #\t)
 				 (value #t))
@@ -177,15 +184,12 @@
 				 (value #t))
 				(debug
 				 (single-char #\d))))))
-    (define (opt tag default)
-      (let ((pair (assq tag options)))
-	(if pair (cdr pair) default)))
 
-    (if (opt 'debug #f)
+    (if (option-ref options 'debug #f)
 	(enable-debug-mode))
 
     (set! test-suite
-	  (or (opt 'test-suite #f)
+	  (or (option-ref options 'test-suite #f)
 	      (getenv "TEST_SUITE_DIR")
 	      default-test-suite))
 
@@ -194,13 +198,8 @@
     ;; not the src-dir.
     (set! tmp-dir (getcwd))
 
-    (let* ((tests
-	    (let ((foo (opt '() '())))
-	      (if (null? foo)
-		  (enumerate-tests test-suite)
-		  foo)))
-	   (log-file
-	    (opt 'log-file "guile.log")))
+    (let* ((tests (enumerate-tests test-suite))
+	   (log-file (option-ref options 'log-file "guile.log")))
 
       ;; Open the log file.
       (let ((log-port (open-output-file log-file)))
@@ -214,7 +213,8 @@
 	  (register-reporter (lambda results
 			       (case (car results)
                                  ((unresolved)
-                                  (and (opt 'flag-unresolved #f)
+                                  (and (option-ref options
+						   'flag-unresolved #f)
                                        (set! global-pass #f)))
 				 ((fail upass error)
 				  (set! global-pass #f)))))


--- orig/test-suite/tests/elisp.test
+++ mod/test-suite/tests/elisp.test
@@ -19,6 +19,9 @@
   :use-module (test-suite lib)
   :use-module (ice-9 weak-vector))
 
+(if #t #t   ;; FIXME:  Ignore this test for now
+  (begin
+
 ;;;
 ;;; elisp
 ;;;
@@ -331,4 +334,6 @@
 
       ))
 
+))
+
 ;;; elisp.test ends here





_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Neil Jerram]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:

> The patch below fixes these two things.  It also comments out the
> `elisp' test which results in a stack overflow here.  My understanding
> is that the Elisp work is almost only due to Neil has never been widely
> used, so hopefully it won't hurt too much until a better solution is
> found.

That's correct.  I'll see if I can debug this, though, so that the
test can then be reinstated.  Thanks for pointing it out.

     Neil



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Neil Jerram]

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
>> The patch below fixes these two things.  It also comments out the
>> `elisp' test which results in a stack overflow here.  My understanding
>> is that the Elisp work is almost only due to Neil has never been widely
>> used, so hopefully it won't hurt too much until a better solution is
>> found.
>
> That's correct.  I'll see if I can debug this, though, so that the
> test can then be reinstated.  Thanks for pointing it out.

Actually I'm not sure I understand.  Do you see the stack overflow
only after applying your patch, or did it occur before the patch also
on your system?  I don't see a problem on Intel using current CVS.

Thanks,
   Neil



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Ludovic Courtès]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> Actually I'm not sure I understand.  Do you see the stack overflow
> only after applying your patch, or did it occur before the patch also
> on your system?  I don't see a problem on Intel using current CVS.

When running `elisp.test' untouched.  Here's what I get:

  $ guile -L .. -l tests/elisp.test
  [...]
  PASS: scheme: value preservation: cdr
  PASS: scheme: value preservation: vector-ref
  ERROR: Stack overflow

Adding `format' expressions shows that this seems to occur when evaluating:

  (if (defined? '%nil)
      (use-modules (lang elisp interface)))

However, it works when run like this:

  $ guile -L ..
  guile> (load "tests/elisp.test")
  PASS: scheme: nil value is a boolean: boolean?
  [...]
  PASS: elisp: (defvar x 4)
  PASS: elisp: x
  guile>

So I tried the following:

  $ guile -L ..  -c '(begin (set! %load-hook (lambda (f) (format #t "loading ~a...~%" f))) (load "tests/elisp.test"))'
  loading tests/elisp.test...
  [...]
  PASS: scheme: value preservation: vector-ref
  loading ../lang/elisp/interface.scm...
  [...]
  PASS: elisp: (defvar x 4)
  PASS: elisp: x

And here everything works fine.

Any idea of the difference between `-l' and `load'?  Looking at
`script.c', that seems equivalent.

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Neil Jerram]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:

> When running `elisp.test' untouched.  Here's what I get:
>
>   $ guile -L .. -l tests/elisp.test
>   [...]
>   PASS: scheme: value preservation: cdr
>   PASS: scheme: value preservation: vector-ref
>   ERROR: Stack overflow

I can't reproduce this, but I wonder if the processing is genuinely on
the border line of the allowed stack depth.  Does it help if you add
this to elisp.test just before the problem:

(debug-set! stack (* (cadr (memq 'stack (debug-options))) 2))

Also, can you confirm whether you see this problem with current CVS,
i.e. without your patch?

> Adding `format' expressions shows that this seems to occur when evaluating:
>
>   (if (defined? '%nil)
>       (use-modules (lang elisp interface)))
>
> However, it works when run like this:
>
>   $ guile -L ..
>   guile> (load "tests/elisp.test")
>   PASS: scheme: nil value is a boolean: boolean?
>   [...]
>   PASS: elisp: (defvar x 4)
>   PASS: elisp: x
>   guile>

On the other hand, if it is a genuine stack depth problem, I'd expect
this one to fail also, since the stack depth of the code for
(top-repl) is a lot more than that of the code that script.c generates
for a -l arg.

> So I tried the following:
>
>   $ guile -L ..  -c '(begin (set! %load-hook (lambda (f) (format #t "loading ~a...~%" f))) (load "tests/elisp.test"))'
>   loading tests/elisp.test...
>   [...]
>   PASS: scheme: value preservation: vector-ref
>   loading ../lang/elisp/interface.scm...
>   [...]
>   PASS: elisp: (defvar x 4)
>   PASS: elisp: x
>
> And here everything works fine.
>
> Any idea of the difference between `-l' and `load'?  Looking at
> `script.c', that seems equivalent.

Yes.  This seems pretty odd.

      Neil



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Ludovic Courtès]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> I can't reproduce this, but I wonder if the processing is genuinely on
> the border line of the allowed stack depth.  Does it help if you add
> this to elisp.test just before the problem:
>
> (debug-set! stack (* (cadr (memq 'stack (debug-options))) 2))

Yes, it does help, i.e. the test now passes even when ran with `-l'.

> Also, can you confirm whether you see this problem with current CVS,
> i.e. without your patch?

My patch (the one that relates to the test-suite) merely comments out
`elisp.test'.  So I just reverted it so that I could actually run the
test, that's it (i.e. it is current CVS).

OTOH, I'm using the "per-module reader" patch I posted sometime ago.  It
(slightly) modifies `primitive-load' in a way that increases the number
of Scheme procedure calls that are necessary to load the various elisp
modules (not dramatically, though -- I guess it just yields an
additional call to `apply' instead of calling `read' directly).

But still, that doesn't explain why the stack overflows when using `-l'
and why everything's fine when loading the test from the REPL.

> On the other hand, if it is a genuine stack depth problem, I'd expect
> this one to fail also, since the stack depth of the code for
> (top-repl) is a lot more than that of the code that script.c generates
> for a -l arg.

Right.

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> But still, that doesn't explain why the stack overflows when using `-l'
> and why everything's fine when loading the test from the REPL.

gdb might show where the stack is being used up, trap on
scm_report_stack_overflow I think.

When I looked it was scm_ceval (or whatever it's called).  That's the
innermost recursion is it?  Maybe someone smart could review to see if
it can be reduced, for the commonest usages.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> It turns out that I had never looked at the test-suite, mostly because
> it isn't run at `make check' time.

It is, but from the top-level ./check-guile.

> Additionally, the `guile-test' script was largely outdated.

Maybe could be removed if no longer used.

> It also comments out the `elisp' test which results in a stack
> overflow here.

Works for me, you might have struck a bug.  But check you're not
compiling with -O0, that used a lot more stack for me with gcc 4 and
overflowed in strange places.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Kevin Ryde]

I wrote:
>
> It is, but from the top-level ./check-guile.

I mean the test-suite is run from make check, but via the top-level
Makefile.am, not the Makefile.am in the test-suite directory.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Ludovic Courtès]

Hi,

Kevin Ryde <user42@zip.com.au> writes:

> It is, but from the top-level ./check-guile.

Ah, I see.

BTW, this script sets `GUILE_LOAD_PATH' to `${top_srcdir}/test-suite'
only.  Consequently, the `ice-9' modules (and in particular
`boot-9.scm') are loaded from `${datadir}/guile/1.7', /not/ from the
source tree, which is wrong.

> Maybe could be removed if no longer used.

The top-level `check-guile' actually uses it.

In fact, I would rather remove `check-guile' and the top-level
Makefile.am's `TESTS', and let `make check' recursively find out what to
do.  This way, things related to the test suite would all be gathered
into the `test-suite' directory.  Additionally, `make check' from there
would really run the whole test suite.

> Works for me, you might have struck a bug.  But check you're not
> compiling with -O0, that used a lot more stack for me with gcc 4 and
> overflowed in strange places.

I did compile at least parts of Guile with `-O0' but I'm using GCC
3.3.5.

BTW, as usual (sigh), here is a new version of the previous patch:  the
previous one disabled the ability to pass a list of test files to
`guile-test'.

Thanks,
Ludovic.


\f
--- orig/test-suite/Makefile.am
+++ mod/test-suite/Makefile.am
@@ -101,3 +101,6 @@
 	  cp -pR $(srcdir)/$$d $(distdir)/$$d; \
           rm -rf $(distdir)/$$d/CVS; \
         done
+
+check-local:
+	./guile-test --test-suite tests/


--- orig/test-suite/guile-test
+++ mod/test-suite/guile-test
@@ -1,5 +1,12 @@
-#!../libguile/guile \
--e main -s
+#!/bin/sh
+# This is in fact -*- Scheme -*- code.
+
+# We need to make sure that we load the right `ice-9' modules so `-L' is not
+# enough since it gets parsed too late.
+GUILE_LOAD_PATH=".."
+export GUILE_LOAD_PATH
+
+exec ../libguile/guile -L .. -l $0 -e main -- "$@"
 !#
 
 ;;;; guile-test --- run the Guile test suite
@@ -177,15 +184,12 @@
 				 (value #t))
 				(debug
 				 (single-char #\d))))))
-    (define (opt tag default)
-      (let ((pair (assq tag options)))
-	(if pair (cdr pair) default)))
 
-    (if (opt 'debug #f)
+    (if (option-ref options 'debug #f)
 	(enable-debug-mode))
 
     (set! test-suite
-	  (or (opt 'test-suite #f)
+	  (or (option-ref options 'test-suite #f)
 	      (getenv "TEST_SUITE_DIR")
 	      default-test-suite))
 
@@ -194,13 +198,13 @@
     ;; not the src-dir.
     (set! tmp-dir (getcwd))
 
-    (let* ((tests
-	    (let ((foo (opt '() '())))
-	      (if (null? foo)
-		  (enumerate-tests test-suite)
-		  foo)))
-	   (log-file
-	    (opt 'log-file "guile.log")))
+    (let* ((tests (let ((files (option-ref options '() '())))
+		    (if (null? files)
+			(enumerate-tests test-suite)
+			(begin
+			  (set! test-suite ".")
+			  files))))
+	   (log-file (option-ref options 'log-file "guile.log")))
 
       ;; Open the log file.
       (let ((log-port (open-output-file log-file)))
@@ -214,7 +218,8 @@
 	  (register-reporter (lambda results
 			       (case (car results)
                                  ((unresolved)
-                                  (and (opt 'flag-unresolved #f)
+                                  (and (option-ref options
+						   'flag-unresolved #f)
                                        (set! global-pass #f)))
 				 ((fail upass error)
 				  (set! global-pass #f)))))


--- orig/test-suite/tests/elisp.test
+++ mod/test-suite/tests/elisp.test
@@ -19,6 +19,9 @@
   :use-module (test-suite lib)
   :use-module (ice-9 weak-vector))
 
+(if #t #t   ;; FIXME:  Ignore this test for now
+  (begin
+
 ;;;
 ;;; elisp
 ;;;
@@ -331,4 +334,6 @@
 
       ))
 
+))
+
 ;;; elisp.test ends here




_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> BTW, this script sets `GUILE_LOAD_PATH' to `${top_srcdir}/test-suite'
> only.  Consequently, the `ice-9' modules (and in particular
> `boot-9.scm') are loaded from `${datadir}/guile/1.7', /not/ from the
> source tree, which is wrong.

Did you try it (ie. ./check-guile)?  I believe it uses
./pre-inst-guile, and that script sets the paths correctly to run
uninstalled.

A typical run to do one test is say "./check-guile elisp.test" from
the top-level.

> In fact, I would rather remove `check-guile' and the top-level
> Makefile.am's `TESTS', and let `make check' recursively find out what to
> do.

Yep.  I've done that for my charting package, you can set
TESTS_ENVIRONMENT to "guile -s" or whatever for setups and so the test
files don't have to be executables.  It's automake-friendly, but you
miss out on the aggregated success/fail counts that the current setup
gives.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Ludovic Courtès]

Hi,

Kevin Ryde <user42@zip.com.au> writes:

> Did you try it (ie. ./check-guile)?  I believe it uses
> ./pre-inst-guile, and that script sets the paths correctly to run
> uninstalled.

This is true, I was wrong.  ;-)

> Yep.  I've done that for my charting package, you can set
> TESTS_ENVIRONMENT to "guile -s" or whatever for setups and so the test
> files don't have to be executables.  It's automake-friendly, but you
> miss out on the aggregated success/fail counts that the current setup
> gives.

Right, but we do not care much about the aggregated counts, do we?
Especially since there are only two test subdirectories.  And I believe
that reducing the complexity of the test scripts and leaving only those
scripts that are useful would be beneficial.

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] Test-suite update

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> Right, but we do not care much about the aggregated counts, do we?

Dunno, a matter of personal preference I guess :-).

Myself I wouldn't mind a --gdb option or something on ./pre-inst-guile
and/or ./check-guile.  The best way I'd found to run up gdb was to
change the exec at the end of pre-inst-guile (to ./libtool gdb ...).


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Socket API improvement, patch #6

[Ludovic Courtès]

Hi,

Kevin Ryde <user42@zip.com.au> writes:

> Beaut, I gave it a bit of a tweak and applied it.

Ok, I'm nitpicking: basically, for function descriptions, I prefer the
style that I used rather than your style.  For instance, I suggested:

  @deftypefn {C Function} {struct sockaddr *}scm_c_make_socket_address (SCM family, SCM address, SCM args, size_t *address_size)
  Return a newly-allocated @code{sockaddr} structure that reflects
  @var{address}, an address of family @var{family}, with the
  family-specific parameters @var{args} (see the description of
  @var{make-socket-address} for details).  On success, a non-@code{NULL}
  pointer is returned and @var{address_size} is updated to the actual
  size (in bytes) of the returned address.  The returned structure must
  eventually be freed using @code{free ()}.
  @end deftypefn

while you preferred:

  @deftypefn {C Function} {struct sockaddr *} scm_c_make_socket_address (SCM family, SCM address, SCM args, size_t *outsize)
  Return a newly-@code{malloc}ed @code{struct sockaddr} created from
  arguments like those taken by @code{scm_make_socket_address} above.

  The size (in bytes) of the @code{struct sockaddr} return is stored
  into @code{*@var{outsize}}.  An application must call @code{free} to
  release the returned structure when no longer required.
  @end deftypefn

IMO, the style you used is "sploppier" and less concise.  The "formal"
declarative style I used is also more consistent with the rest of the
manual and other GNU manuals as well I think.

Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Socket API improvement, patch #6

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> Ok, I'm nitpicking:

My rationale below.

>   structure that reflects

"reflects" is not a good word.

>   @var{address}, an address of family @var{family}, with the
>   family-specific parameters @var{args} (see the description of
>   @var{make-socket-address} for details).

I felt all this could be left as a ref to scm_make_socket_address.

>   On success, a non-@code{NULL}

It's always non-NULL, no need to say that.

>   @var{address_size} is updated

I changed the name and tried to make it clearer that it's an output.
"updated" might make you think it was some value then changed to
another.

>   The returned structure must eventually be freed using
>   @code{free ()}.

I thought "eventually" was a clumsy expression.  What I replaced it
with might stand further polish though.

> "formal" declarative style

A manual is not a specification (fortunately), so there's no need to
be overly formal.

A manual, even a reference manual, is essentially a teaching tool, you
want a programmer reading it to learn what a func does, or refresh
their memory of what it does.  On that basis some informality in
expression is fine, though you definitely need the ideas presented in
a nice logical sequence.  (Which for me means the key point or points
first, followed by gory details or exceptions.)


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Socket API improvement, patch #6

[Ludovic Courtès]

Hi Kevin,

I mostly agree with your remarks about the piece of documentation I
included---these were mostly errors unrelated to the point I was trying
to make.  :-)

Kevin Ryde <user42@zip.com.au> writes:

> A manual is not a specification (fortunately), so there's no need to
> be overly formal.

Agreed.  But you missed my point: the wording has to be consistent
across the manual (or, even better, across GNU manuals) and it has to be
non-ambiguous.

For instance, a sentence like "Return _a_ Scheme object." doesn't give a
lot of information.  What I like in GNU reference manuals is that
function descriptions usually start with a single sentence (or a couple
of sentences) that describes the return value and each argument.
Example:

   Apply PROC to each element of the list ARG1 (if only two arguments
   are given), or to the corresponding elements of the argument lists
   (if more than two arguments are given).  The result(s) of the
   procedure applications are saved and returned in a list.

Or (glibc):

   The `open' function creates and returns a new file descriptor for
   the file named by FILENAME. 

> A manual, even a reference manual, is essentially a teaching tool

I disagree: a tutorial is teaching material, a reference manual is,
well, a reference documentation.  Of course, a manual may very well
include a tutorial alongside the reference documentation: that's what
Guile's manual does in the nodes other than `API Reference' and `Guile
Modules'.

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel