[patch v1] fgets: more tests

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> I like this... but it's unstructured?

It's debug info for the *.out file, just there in case the test fails
for someone but is unreproducible, the data in the *.out file might help
fix the bug.  Did you want more structure in the syntax (for parsing the
*.out files), or in the way it's generated?

>> +  printf ("testing base operation...");
>
> I dislike that we are printing unstructured information about the test?
>
> Can we put this into TEST_NAME ("testing base operation...")
>
> And return a test handle?

For what purpose?  Keeping track of counts of tests/fails, only to write
them in a file that nobody will look at? ;-)

>> +  f = my_open ("hello\n", 6, "r");
>> +  memset (buf, 0x11, sizeof (buf));
>> +  str = fgets (buf, 100, f);
>> +  if (str == NULL)
>> +    {
>> +      FAIL ("FAIL (returned NULL)\n");
>
> ... this is linked to a sub-test result.
>
>> +    }
>> +  else if (memcmp (str, "hello\n\0", 7) != 0)
>> +    {
>> +      FAIL ("FAIL (returned %s instead of %s)\n", hex (str, 7), hex ("hello\n\0", 7));
>
> ... this is linked to a sub-test result.
>
>> +    }
>> +  else if (bytes_read != 6)
>> +    {
>> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 6);
>
> ... this is linked to a sub-test result.
>
>> +    }
>> +  else
>> +    printf ("PASS\n");
>
> I dislike that we are printing unstructured PASS information to stdout.

Note that the first printf doesn't have a \n so this goes on the same
line as the test name.

Is there any harm in mentioning that a subtest passed?

> Do we need sub-test counting for pass/fail in support/*?
>
> I'd hope we could say PASS ("test name"); ?

If we used that paradigm for the fails, a segfaulting test wouldn't
print the test name before truncating the *.out file (assuming stdout is
unbuffered).  So we can't use it for the pass's either.

If the idea is to collect metrics about sub-tests, I have to ask "for
who?"  That's a lot of work for only some of the tests, for a file
nobody will look at unless the test fails...

But to see what it would look like, I added a PASS() macro to
tst-fgets2.c, and replaced "PASS" with "ok" to avoid confusion in the
output (which prints "error" for FAIL()).  So you get a *.out file like
this:

testing base operation...ok
testing zero size file...ok
testing zero size buffer...error: tst-fgets2.c:238: 7 bytes read instead of 0
testing NULL buffer...ok
testing embedded NUL...ok
testing writable stream...ok
testing closed fd stream...ok

> The alternative is we modify TEST_VERIFY e.g.
>
> TEST_VERIFY_PF (<assertion>,
>                 "pass message",
>                 "fail messsage")

The problem with the VERIFY() macros is that they print the expression
being verified, not the data being used by the expression.  I.e. having
this in a *.out file is not that useful:

FAIL: i != j

But something like this is much more useful:

FAIL: found 0x45 not same as expected 0x46

It's very hard to macro-ize this type of output as the format depends on
the type of data and expression being tested.

>> +  else if (bytes_read != 0)
>> +    {
>> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
>
> Why doesn't this use FAIL?

Fixed.  I tweaked the message to fit better with what FAIL() emits.

>> +  else if (bytes_read != 0)
>> +    {
>> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
>
> Why doesn't this use FAIL?

Fixed.

Andrew Pinski <pinskia@gmail.com> wrote:
> This seems not related to the version of GCC but rather if
> _FORTIFY_SOURCE is defined or not.

I'm using gcc 11, and added -D_FORTIFY_SOURCE=2 to the build, but
couldn't reproduce this.  Nevertheless, I wrapped the includes in the
macros in hopes to fix it.

V2 to follow, hopefully CI will like it better.