Write errors to stdout and not stderr in wcsmbs/tst-mbrtowc.c
diff mbox

Message ID 20141003205833.6deb3569@zion
State New
Headers show

Commit Message

Arjun Shankar Oct. 3, 2014, 6:58 p.m. UTC
tst-mbrtowc.c uses `assert' for tests, which writes to stderr. This
patch fixes that using dup2, and gets tst-mbrtowc.c and tst-wcrtomb.c
to use the test skeleton - which is just generally nice.

ChangeLog:

2014-10-03  Arjun Shankar  <arjun.is@lostca.se>

	* wcsmbs/tst-mbrtowc.c: Write errors to stdout, use skeleton.
	* wcsmbs/tst-wcrtomb.c: Use skeleton.
---
 wcsmbs/tst-mbrtowc.c | 13 ++++++++++---
 wcsmbs/tst-wcrtomb.c |  7 +++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

Comments

Roland McGrath Oct. 3, 2014, 7:36 p.m. UTC | #1
The dup2 hack is either cute or horrible (likely both).  I don't think we
should do anything like that in one random test.  Rather, we should settle
on a uniform policy for tests and using assert.  It could be that we never
use assert.  Or it could be that we use assert freely and just make
test-skeleton.c always do the dup2 hack itself.  Or it could be something
else I haven't thought of off hand.  But let's get a plan.

If you'd like to convert these tests to use test-skeleton.c first as a
separate change before we figure out how to address the assert issue,
that could go in right away.


Thanks,
Roland
Arjun Shankar Oct. 3, 2014, 9:05 p.m. UTC | #2
> The dup2 hack is either cute or horrible (likely both).  I don't think we
> should do anything like that in one random test.

Okay.

>  Rather, we should settle
> on a uniform policy for tests and using assert.  It could be that we never
> use assert.  Or it could be that we use assert freely and just make
> test-skeleton.c always do the dup2 hack itself.  Or it could be something
> else I haven't thought of off hand.  But let's get a plan.

One thing we cannot do is use `dup2' in the skeleton. It might confuse tests
that specifically test for output written to stdout/stderr. One such example
is 'posix/tst-getopt_long1.c'.

> If you'd like to convert these tests to use test-skeleton.c first as a
> separate change before we figure out how to address the assert issue,
> that could go in right away.

Okay. And I see that this itself is a task in the master TODO [1]. I will
get on it more thoroughly than just these two tests. In the meanwhile,
there are also a lot of tests writing to stderr [2]. If we have the policy
on that wrapped up, I will work on it next.

I am sort of new here, by the way. Hello, glibc hackers!

Cheers,
Arjun

[1] https://sourceware.org/glibc/wiki/Development_Todo/Master#Use_test-skeleton.c
[2] http://fpaste.org/139008/
Roland McGrath Oct. 3, 2014, 9:17 p.m. UTC | #3
> One thing we cannot do is use `dup2' in the skeleton. It might confuse tests
> that specifically test for output written to stdout/stderr. One such example
> is 'posix/tst-getopt_long1.c'.

If need be, we could make it depend on some macros you define before
including the file.  In the one case you cite, it would not break anything.
That test uses freopen to redirect stderr at the stdio level, so it never
writes to fd 2 at all.  So it might well work as a blanket change, but
certainly we should think about it more first.

> > If you'd like to convert these tests to use test-skeleton.c first as a
> > separate change before we figure out how to address the assert issue,
> > that could go in right away.
> 
> Okay. And I see that this itself is a task in the master TODO [1]. I will
> get on it more thoroughly than just these two tests. 

That would be a very fine contribution and there shouldn't be much of
anything to figure out or discuss to get it done.

> In the meanwhile, there are also a lot of tests writing to stderr [2]. If
> we have the policy on that wrapped up, I will work on it next.

> I am sort of new here, by the way. Hello, glibc hackers!

Welcome!  We very much appreciate new contributors.  Have you completed
your copyright assignment paperwork for future contributions to libc?
I don't see your name in copyright.list.  
See https://sourceware.org/glibc/wiki/Contribution%20checklist for
everything you should know.


Thanks,
Roland
Siddhesh Poyarekar Oct. 4, 2014, 3:15 a.m. UTC | #4
On 4 October 2014 02:47, Roland McGrath <roland@hack.frob.com> wrote:
> Welcome!  We very much appreciate new contributors.  Have you completed
> your copyright assignment paperwork for future contributions to libc?
> I don't see your name in copyright.list.
> See https://sourceware.org/glibc/wiki/Contribution%20checklist for
> everything you should know.

Arjun works for Red Hat, so the blanket copyright assignment ought to cover him.

Siddhesh
Will Newton Oct. 6, 2014, 8:33 a.m. UTC | #5
On 3 October 2014 20:36, Roland McGrath <roland@hack.frob.com> wrote:
> The dup2 hack is either cute or horrible (likely both).  I don't think we
> should do anything like that in one random test.  Rather, we should settle
> on a uniform policy for tests and using assert.  It could be that we never
> use assert.  Or it could be that we use assert freely and just make
> test-skeleton.c always do the dup2 hack itself.  Or it could be something
> else I haven't thought of off hand.  But let's get a plan.

One option could be to use a hand-rolled assert function which is what
most unit testing libraries do. This would also give scope for adding
more esoteric asserts that can output more detail about the failure.
Siddhesh Poyarekar Oct. 6, 2014, 8:54 a.m. UTC | #6
On Mon, Oct 06, 2014 at 09:33:33AM +0100, Will Newton wrote:
> On 3 October 2014 20:36, Roland McGrath <roland@hack.frob.com> wrote:
> > The dup2 hack is either cute or horrible (likely both).  I don't think we
> > should do anything like that in one random test.  Rather, we should settle
> > on a uniform policy for tests and using assert.  It could be that we never
> > use assert.  Or it could be that we use assert freely and just make
> > test-skeleton.c always do the dup2 hack itself.  Or it could be something
> > else I haven't thought of off hand.  But let's get a plan.
> 
> One option could be to use a hand-rolled assert function which is what
> most unit testing libraries do. This would also give scope for adding
> more esoteric asserts that can output more detail about the failure.

Agreed, but to be clear, the hand-rolled assert should always exit
normally, i.e. not with an abort.  An abort precludes any cleanups
that may be necessary for a test.  In fact asserts that result in such
abnormal termination should be actively discouraged.

On whether we should redirect stderr to stdout, I am not very excited
about it.  I don't want to exclude the possibility of wanting to use
stderr some time in future for the tests.

Siddhesh

Patch
diff mbox

diff --git a/wcsmbs/tst-mbrtowc.c b/wcsmbs/tst-mbrtowc.c
index 3e1eb72..43601cb 100644
--- a/wcsmbs/tst-mbrtowc.c
+++ b/wcsmbs/tst-mbrtowc.c
@@ -24,6 +24,7 @@ 
 #include <stdlib.h>
 #include <string.h>
 #include <wchar.h>
+#include <unistd.h>
 
 
 static int check_ascii (const char *locname);
@@ -139,7 +140,7 @@  utf8_test (void)
 
   if (!setlocale (LC_CTYPE, locale))
     {
-      fprintf (stderr, "locale '%s' not available!\n", locale);
+      printf ("locale '%s' not available!\n", locale);
       exit (1);
     }
 
@@ -151,11 +152,14 @@  utf8_test (void)
 }
 
 
-int
-main (void)
+static int
+do_test (void)
 {
   int result = 0;
 
+  /* Failed `assert' messages should go to stdout, not stderr.  */
+  dup2 (STDOUT_FILENO, STDERR_FILENO);
+
   /* Check mapping of ASCII range for some character sets which have
      ASCII as a subset.  For those the wide char generated must have
      the same value.  */
@@ -230,3 +234,6 @@  check_ascii (const char *locname)
 
   return res != 0;
 }
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/wcsmbs/tst-wcrtomb.c b/wcsmbs/tst-wcrtomb.c
index 3f052f3..cad343e 100644
--- a/wcsmbs/tst-wcrtomb.c
+++ b/wcsmbs/tst-wcrtomb.c
@@ -26,8 +26,8 @@ 
 static int check_ascii (const char *locname);
 
 
-int
-main (void)
+static int
+do_test (void)
 {
   int result = 0;
 
@@ -92,3 +92,6 @@  check_ascii (const char *locname)
 
   return res != 0;
 }
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"