time/tst-strptime2.c: test full input range -9999 to +9999

Message ID 1439513365-24723-1-git-send-email-james@loowit.net
State Superseded
Headers

Commit Message

James Perkins Aug. 14, 2015, 12:49 a.m. UTC
  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.

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).

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.
  * In 10020 of the runs, strptime returns failure (NULL) because the
    string was an invalid format.

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.

Signed-off-by: James Perkins <james@loowit.net>
---
 time/tst-strptime2.c | 190 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 137 insertions(+), 53 deletions(-)
  

Comments

Mike Frysinger Aug. 14, 2015, 1:29 a.m. UTC | #1
On 13 Aug 2015 17:49, James Perkins wrote:
> +/*
> +   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.
> +   */

GNU style says the first & last lines should be cuddled.  e.g.
/* Write a string ....
   part of the time is outside the valid range of 00 to 59.  */

> +	sprintf(buf, "%s %c", dummy_string, sign);

GNU style says there should be a space before the (

> +      printf ("%s: tm_gmtoff is %ld\n", buf, (long int) tm.tm_gmtoff);

tm_gmtoff is already long int, so why the cast ?  i know it was there before,
but the question remains.

> +static int
> +do_test (void)

this could do with a --verbose option i think that'd dump every buffer tested
and the results of the test.  trusting silent output does the right thing vs
getting a verbose report and scanning by eye/grep/whatever makes me way more
confident and helps check for future regressions.

look at the CMDLINE_OPTIONS & CMDLINE_PROCESS defines in test-skeleton.c.
-mike
  
Carlos O'Donell Aug. 14, 2015, 2:44 p.m. UTC | #2
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.
  
James Perkins Aug. 15, 2015, 6:43 p.m. UTC | #3
Thanks for your patient and prompt review, Mike.

I'm putting my tst-strptime2.c rework into a patch following the one that
repairs strptime_l.c's %z issues, and calling the patchset V5 - I shall post
it momentarily.

On Thu, Aug 13, 2015 at 6:29 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 13 Aug 2015 17:49, James Perkins wrote:
>> +/*
>> +   Write a string into the supplied buffer, containing a dummy string,
...
>> +   part of the time is outside the valid range of 00 to 59.
>> +   */
>
> GNU style says the first & last lines should be cuddled.  e.g.
> /* Write a string ....
>    part of the time is outside the valid range of 00 to 59.  */
>
>> +     sprintf(buf, "%s %c", dummy_string, sign);
>
> GNU style says there should be a space before the (

I've addressed the GNU C style issues you raised, and ran my code through
indent, which fixed up a few more.

>> +      printf ("%s: tm_gmtoff is %ld\n", buf, (long int) tm.tm_gmtoff);
>
> tm_gmtoff is already long int, so why the cast ?  i know it was there before,
> but the question remains.

I'm not sure why this cast was here historically. I will remove it -- as you
point out, it is unnecessary.

>> +static int
>> +do_test (void)
>
> this could do with a --verbose option i think that'd dump every buffer tested
> and the results of the test.  trusting silent output does the right thing vs
> getting a verbose report and scanning by eye/grep/whatever makes me way more
> confident and helps check for future regressions.
>
> look at the CMDLINE_OPTIONS & CMDLINE_PROCESS defines in test-skeleton.c.

That is a great suggestion; I had been adding verbose code and then removing
it prior to sending the patch, and this is a way that it can stay and remain an
aid to people debugging strptime behavior. I've worked that in, too.

Cheers,
James
  
James Perkins Aug. 15, 2015, 6:56 p.m. UTC | #4
On Fri, Aug 14, 2015 at 7:44 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/13/2015 08:49 PM, James Perkins wrote:
>> 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.

Carlos, thanks for the review. In the upcoming V5 patchset I've done what you've
asked for.

>>  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.

By the way, I noticed my iteration from -9959 to 9959 was getting its sign byte
by looking at the value of the two's-complement int argument to mkbuf().
This meant my tests were missing the -0000, -000, -00, -0 and - input string
cases. I changed mkbuf to pass in sign information in a separate argument
so that it now tests the negative 0 cases as well.

>> -  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;
>> +
>> +    }

I've simplifed the above to just print the string using sprint, including
all four digits, and then truncate the string to the correct length using
ndigits. This simplifies logic throughout greatly.

>> +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.

I am able to turn most of the above into nested for loops:

for ndigits from 0 to 4
  for hhmm from 0 to 9999 by appropriate step based on ndigits
    for sign from positive to negative
      gmtoff = mkbuf(buf, hhmm, ndigits);
      result |= compare (buf, gmtoff)

... making it much easier to read and clearer.

Patch set forthcoming...

Cheers,
James
  

Patch

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;
 
-  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;
+}
+
+
+/* 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;
+}
+
+
+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);
+    }
 
   return result;
 }