Bad handling of locales in test-suite

Bad handling of locales in test-suite

[Mark H Weaver]

Hello all,

While investigating a test failure in "foreign.test" that happens when
running "./check-guile" but not when running "make check" or
"./check-guile foreign.test", I've discovered that our handling of
locales in the Guile test suite is a mess.

It turns out that this particular test passes or fails depending on the
locale, and that several *.test files set the locale without restoring
it, so the locale setting changes from one *.test file to the next.

The handling of locales in our test suite is quite inconsistent:

* In some files that set the locale, there's broken code that tries to
  restore the locale to its previous value, but fails to do so.  The
  author apparently believed that (setlocale LC_ALL x) returns the
  previous locale, when it fact it returns the new locale:

    encoding-iso88591.test
    encoding-iso88597.test
    encoding-utf8.test
    srfi-14.test

* In several other places, the locale is set without even trying to
  restore it:

    bytevectors.test
    regexp.test
    i18n.test
    format.test
    srfi-19.test
    tree-il.test

* Some files check to see if 'setlocale' is defined before calling it,
  while other files call 'setlocale' unconditionally.

* foreign.test: "%default-port-conversion-strategy is substitute"
  fails on modern glibc if the locale is "en_US.UTF-8", because "ĥ" is
  transliterated to "h", but works as expected if the locale is set to
  "C", where "ĥ" becomes "?".

It's easy enough to fix these problems: the "test-suite/guile-test"
script, which loads the requested *.test files (or all of them), could
be modified to save and restore the locale, or it could simply set the
locale to a known value before each test file.

However, there's a question of policy to decide.  Here are some options:

* (setlocale LC_ALL "C") before loading each file.  The argument for
  this would be consistency, so that the test suite runs the same way
  regardless of the user's locale.

* (setlocale LC_ALL "") before loading each file.  The argument for this
  would be diversity, to make sure that Guile works as expected for any
  locale.

* Save the locale before loading each file, and restore after.

And then there's the question of what to do about all the places where
the locale is set in the individual *.test files.  Should they be left
mostly as-is, or should we clean up and remove all the 'setlocale' calls
that would be rendered redundant with whatever policy is chosen above?

I'm leaning toward (setlocale LC_ALL "C") before loading each file, and
stripping out all (setlocale LC_ALL "C") calls in the individual test
files, as well as the broken attempts to restore the previous locale
setting.

What do you think?

      Mark

Re: Bad handling of locales in test-suite

[Ludovic Courtès]

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

Mark H Weaver <mhw@netris.org> skribis:

> While investigating a test failure in "foreign.test" that happens when
> running "./check-guile" but not when running "make check" or
> "./check-guile foreign.test", I've discovered that our handling of
> locales in the Guile test suite is a mess.

I’d call it “pragmatic”.  :-)

> The handling of locales in our test suite is quite inconsistent:
>
> * In some files that set the locale, there's broken code that tries to
>   restore the locale to its previous value, but fails to do so.  The
>   author apparently believed that (setlocale LC_ALL x) returns the
>   previous locale, when it fact it returns the new locale:
>
>     encoding-iso88591.test
>     encoding-iso88597.test
>     encoding-utf8.test
>     srfi-14.test
>
> * In several other places, the locale is set without even trying to
>   restore it:
>
>     bytevectors.test
>     regexp.test
>     i18n.test
>     format.test
>     srfi-19.test
>     tree-il.test

I’d recommend the latter approach: before running tests for which the
locale matters, just set it.  I wouldn’t bother restoring the previous
locale or anything.

> * Some files check to see if 'setlocale' is defined before calling it,
>   while other files call 'setlocale' unconditionally.

These should probably use ‘with-locale’.

> * foreign.test: "%default-port-conversion-strategy is substitute"
>   fails on modern glibc if the locale is "en_US.UTF-8", because "ĥ" is
>   transliterated to "h", but works as expected if the locale is set to
>   "C", where "ĥ" becomes "?".

This test was supposed to be locale-insensitive, but I didn’t know
transliteration was sensitive to the language-part of the locale.  How
about this patch?


[-- Attachment #2: Type: text/x-patch, Size: 931 bytes --]

diff --git a/test-suite/tests/foreign.test b/test-suite/tests/foreign.test
index acdb3db..ef1bdba 100644
--- a/test-suite/tests/foreign.test
+++ b/test-suite/tests/foreign.test
@@ -188,12 +188,16 @@
   (pass-if "%default-port-conversion-strategy is substitute"
     (let ((s "teĥniko")
           (member (negate (negate member))))
+      ;; Transliteration depends on the language part of the locale.
+      ;; For instance, on glibc 2.18, the transliteration of 'ĥ' is 'h'
+      ;; in en_US.utf8, whereas it is '?' in C.
+      (with-locale "C"
         (member (with-fluids ((%default-port-conversion-strategy 'substitute))
                   (pointer->string (string->pointer s "ISO-8859-1")))
                 '("te?niko"
 
                   ;; This form is found on FreeBSD 8.2 and Darwin 10.8.0.
-                "te^hniko"))))
+                  "te^hniko")))))
 
   (pass-if "bijection"
     (let ((s "hello, world"))

[-- Attachment #3: Type: text/plain, Size: 711 bytes --]


> However, there's a question of policy to decide.  Here are some options:
>
> * (setlocale LC_ALL "C") before loading each file.  The argument for
>   this would be consistency, so that the test suite runs the same way
>   regardless of the user's locale.
>
> * (setlocale LC_ALL "") before loading each file.  The argument for this
>   would be diversity, to make sure that Guile works as expected for any
>   locale.
>
> * Save the locale before loading each file, and restore after.

[...]

> I'm leaning toward (setlocale LC_ALL "C") before loading each file,

Me too.  That way, things would be deterministic.  There are tests that
exercise specific locales when that’s what we want.

Thanks,
Ludo’.