Re: [PATCH] time/tst-strptime2.c: test full input range -9999 to +9999

["Carlos O'Donell" <carlos at redhat dot com>]

On 08/13/2015 08:49 PM, James Perkins wrote:
> strptime's %z specifier parses a string consisting of a sign ('+'
> or '-'), two hours digits, and optionally two minutes digits, into
> a tm.gmtoff field containing the signed number of seconds the time
> zone is offset from UTC time.
> 
> The time/tst-strptime2.c file passes a short list of strings through
> strptime, validating that either the gmtoff value returned matches
> an expected value, or that strptime returns an expected NULL for
> invalid strings (for example, when the minutes portion of the string
> is outside of the range 00 to 59, or the sign is missing before the
> hours digits).
> 
> In review of strptime fixes, Carlos O'Donell expressed a wish that
> the test function iterate through the entire range of all possible
> numeric strings (-9999 to +9999) which could be passed to strptime
> %z, and validate the correct response.
> 
> Specifically, the test will look for a NULL response from strptime
> when:
> 
>   * sign ('+' or '-') exists before the first digit (invalid
>     format).
>   * A sign and no digits are found (invalid format).
>   * A sign and one digit are found (invalid format).
>   * A sign and three digits are found (invalid format).
>   * A sign and four digits (-9999 to +9999) are found but the last
>     two digits (minutes) are in the range 60 to 99.

OK.

> The test will look for a success response from strptime with tm.gmtoff
> matching the calculated gmtoff value when:
> 
>   * A sign and four digits are found (-9999 to +9999), and the last
>     two digits (minutes) are in the range 00 to 59.
>   * A sign and two digit strings are found (-99 to +99).

OK.

> The test's iteration over the possible digit values results in 22218
> test runs calculated by the test, run, and passed by strptime:
> 
>   * In 12198 of the runs, strptime returns successfully with gmtoff
>     matching the expected value.

Great.

>   * In 10020 of the runs, strptime returns failure (NULL) because the
>     string was an invalid format.

Perfect.

> 2015-08-13  James Perkins  <james@loowit.net>
> 
> 	* time/tst-strptime2.c (tests): Replace short list of test
> 	strings for strptime %z specifier with code that calculates
> 	every combination of digits. Add tests for 0, 1, and 3 digit
> 	invalid strings, which strptime correctly rejects.
> 

This test looks great to me. I think the following next steps are
required:

- Add a verbose option.
- Fixup GNU-style formatting issues.
- Repost fix + test case changes as "v3"
- I'll do a final review and checkin.

> Signed-off-by: James Perkins <james@loowit.net>
> ---
>  time/tst-strptime2.c | 190 +++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 137 insertions(+), 53 deletions(-)
> 
> diff --git a/time/tst-strptime2.c b/time/tst-strptime2.c
> index 4db8321..8ea8b14 100644
> --- a/time/tst-strptime2.c
> +++ b/time/tst-strptime2.c
> @@ -2,72 +2,156 @@
>  #include <stdio.h>
>  #include <time.h>
>  
> +/* Dummy string is used to match strptime's %s specifier.  */
> +
> +static const char const *dummy_string = "1113472456";
> +
> +/* buffer_size contains the maximum test string length,
> +   including trailing NUL.  */
> +
> +enum {
> +  buffer_size = 20,
> +};
>  
> -static const struct
> -{
> -  const char *fmt;
> -  long int gmtoff;
> -} tests[] =
> -  {
> -    { "1113472456 +1000", 36000 },
> -    { "1113472456 -1000", -36000 },
> -    { "1113472456 +10", 36000 },
> -    { "1113472456 -10", -36000 },
> -    { "1113472456 +1030", 37800 },
> -    { "1113472456 -1030", -37800 },
> -    { "1113472456 +0030", 1800 },
> -    { "1113472456 -0030", -1800 },
> -    { "1113472456 +1157", 43020 },
> -    { "1113472456 +1158", 43080 },
> -    { "1113472456 +1159", 43140 },
> -    { "1113472456 +1200", 43200 },
> -    { "1113472456 -1200", -43200 },
> -    { "1113472456 +1201", 43260 },
> -    { "1113472456 -1201", -43260 },
> -    { "1113472456 +1330", 48600 },
> -    { "1113472456 -1330", -48600 },
> -    { "1113472456 +1400", 50400 },
> -    { "1113472456 +1401", 50460 },
> -    { "1113472456 -2459", -89940 },
> -    { "1113472456 -2500", -90000 },
> -    { "1113472456 +2559", 93540 },
> -    { "1113472456 +2600", 93600 },
> -    { "1113472456 -99", -356400 },
> -    { "1113472456 +99", 356400 },
> -    { "1113472456 -9959", -359940 },
> -    { "1113472456 +9959", 359940 },
> -    { "1113472456 -1060", LONG_MAX },
> -    { "1113472456 +1060", LONG_MAX },
> -    { "1113472456  1030", LONG_MAX },
> -  };
> -#define ntests (sizeof (tests) / sizeof (tests[0]))
>  
> +/*
> +   Write a string into the supplied buffer, containing a dummy string,
> +   + or - sign, two hour digits, optionally two minutes digits,
> +   and trailing NUL.
> +
> +   Also, calculate and return expected results for this value.  If the
> +   input is valid then the expected gmtoffset is returned. If the
> +   value is invalid input to strptime, then LONG_MAX is returned.
> +   LONG_MAX indicates the expectation that strptime will return NULL;
> +   for example, if the number of digits are not correct, or minutes
> +   part of the time is outside the valid range of 00 to 59.
> +   */
>  
>  static int
> -do_test (void)
> +mkbuf (char *buf, int hhmm_signed, size_t ndigits)
>  {
> -  int result = 0;
> +  const int mm_max = 59;
> +  int neg = hhmm_signed < 0 ? 1 : 0;
> +  unsigned int hhmm = neg ? -hhmm_signed : hhmm_signed;
> +  unsigned int hh = hhmm / 100;
> +  unsigned int mm = hhmm % 100;
> +  char sign = neg ? '-' : '+';
> +  int gmtoff_signed = LONG_MAX;

Looks good. It's probably simplest to just use LONG_MAX like this than
anything more complicated.

>  
> -  for (int i = 0; i < ntests; ++i)
> +  switch (ndigits)
>      {
> -      struct tm tm;
> +      case 0:
> +	sprintf(buf, "%s %c", dummy_string, sign);
> +	break;
>  
> -      if (strptime (tests[i].fmt, "%s %z", &tm) == NULL)
> -	{
> -	  if (tests[i].gmtoff != LONG_MAX)
> -	    {
> -	      printf ("round %d: strptime unexpectedly failed\n", i);
> -	      result = 1;
> -	    }
> -	  continue;
> -	}
> +      case 1:
> +      case 2:
> +	sprintf(buf, "%s %c%0*u", dummy_string, sign, ndigits, hh);
> +	break;
> +
> +      case 3:
> +      case 4:
> +	sprintf(buf, "%s %c%0*u", dummy_string, sign, ndigits, hhmm);
> +	break;
> +
> +      default:
> +	sprintf(buf, "%s %c%u", dummy_string, sign, hhmm);
> +	break;
> +
> +    }
> +
> +  if (mm <= mm_max && (ndigits == 2 || ndigits == 4))
> +    {
> +      int gmtoff = hh * 3600 + mm * 60;
>  
> -      if (tm.tm_gmtoff != tests[i].gmtoff)
> +      gmtoff_signed = neg ? -gmtoff : gmtoff;
> +    }
> +
> +  return gmtoff_signed;
> +}

OK.

> +
> +
> +/* Using buffer, test strptime against expected gmtoff result.  */
> +
> +static int
> +compare (const char *buf, int gmtoff)
> +{
> +  int result = 0;
> +  struct tm tm;
> +
> +  if (strptime (buf, "%s %z", &tm) == NULL)
> +    {
> +      if (gmtoff != LONG_MAX)
>  	{
> -	  printf ("round %d: tm_gmtoff is %ld\n", i, (long int) tm.tm_gmtoff);
> +	  printf ("%s: strptime unexpectedly failed\n", buf);
>  	  result = 1;
>  	}
>      }
> +  else if (tm.tm_gmtoff != gmtoff)
> +    {
> +      printf ("%s: tm_gmtoff is %ld\n", buf, (long int) tm.tm_gmtoff);
> +      result = 1;
> +    }
> +
> +  return result;
> +}

OK.

> +
> +
> +static int
> +do_test (void)
> +{
> +  const int hhmm_min = -9999;
> +  const int hhmm_max = 9999;
> +  const int hh_min = -99;
> +  const int hh_max = 99;
> +  int result = 0;
> +  int gmtoff;
> +  char buf[buffer_size];
> +
> +  /* Test sign missing, four digits string (invalid format).  */
> +
> +  sprintf(buf, "%s  1030", dummy_string);
> +  gmtoff = LONG_MAX;
> +  result |= compare(buf, gmtoff);
> +
> +  /* Test sign and no digits strings (invalid format).  */
> +
> +  gmtoff = mkbuf(buf, 0, 0);
> +  result |= compare(buf, gmtoff);
> +
> +  /* Test sign and one digit strings (invalid format).  */
> +
> +  for (int hh = hh_min / 10; hh <= hh_max / 10; hh++)
> +    {
> +      gmtoff = mkbuf(buf, hh * 100, 1);
> +      result |= compare(buf, gmtoff);
> +    }
> +
> +  /* Test sign and two digits strings (valid format).  */
> +
> +  for (int hh = hh_min; hh <= hh_max; hh++)
> +    {
> +      gmtoff = mkbuf(buf, hh * 100, 2);
> +      result |= compare(buf, gmtoff);
> +    }
> +
> +  /* Test sign and three digits strings (invalid format).  */
> +
> +  for (int hhmm = hhmm_min / 10; hhmm <= hhmm_max / 10; hhmm++)
> +    {
> +      gmtoff = mkbuf(buf, hhmm, 1);
> +      result |= compare(buf, gmtoff);
> +    }
> +
> +  /* Test sign and four digits strings. This includes cases
> +     where minutes are in the range 0 to 59 (valid), and
> +     where minutes are in the range 60 to 99 (invalid).  */
> +
> +  for (int hhmm = hhmm_min; hhmm <= hhmm_max; hhmm++)
> +    {
> +      gmtoff = mkbuf(buf, hhmm, 4);
> +      result |= compare(buf, gmtoff);
> +    }

Looks great.

>  
>    return result;
>  }
> 

Cheers,
Carlos.