diff mbox series

Increase the timeout of locale/tst-localedef-path-norm

Message ID 20220718134208.969938-1-mjw@redhat.com
State Rejected
Headers
Series Increase the timeout of locale/tst-localedef-path-norm |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Mark Wielaard July 18, 2022, 1:42 p.m. UTC 
  From: Mark Wielaard <mark@klomp.org>

On the current AArch64 buildbot glibc-fedora-arm64
the 30s timeout is not enough to run this test, the
machine has 8 cores, but the testcase is deliberately
single threaded

  make test t=locale/tst-localedef-path-norm

takes about 45s, so I increased the timeout to 60s.

This is the only failure on this buildbot builder.
https://builder.sourceware.org/buildbot/#/builders/glibc-fedora-arm64
---
 locale/tst-localedef-path-norm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Adhemerval Zanella July 18, 2022, 7:50 p.m. UTC | #1 
On 18/07/22 10:42, Mark Wielaard via Libc-alpha wrote:
> From: Mark Wielaard <mark@klomp.org>
> 
> On the current AArch64 buildbot glibc-fedora-arm64
> the 30s timeout is not enough to run this test, the
> machine has 8 cores, but the testcase is deliberately
> single threaded
> 
>   make test t=locale/tst-localedef-path-norm
> 
> takes about 45s, so I increased the timeout to 60s.
> 
> This is the only failure on this buildbot builder.
> https://builder.sourceware.org/buildbot/#/builders/glibc-fedora-arm64
> ---
>  locale/tst-localedef-path-norm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/locale/tst-localedef-path-norm.c b/locale/tst-localedef-path-norm.c
> index 68995a415..80e1c556d 100644
> --- a/locale/tst-localedef-path-norm.c
> +++ b/locale/tst-localedef-path-norm.c
> @@ -236,5 +236,5 @@ do_test (void)
>    return 0;
>  }
>  
> -#define TIMEOUT 30
> +#define TIMEOUT 60
>  #include <support/test-driver.c>

I think it would be maybe better to either split the test, so we can
improve make check parallel runtime; or use a large TIMEOUTFACTOR.

As a side note, we also added some string tests that require a lot
time so it would be good to reevaluate them for next release.
Carlos O'Donell July 19, 2022, 2:45 a.m. UTC | #2 
On 7/18/22 15:50, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 18/07/22 10:42, Mark Wielaard via Libc-alpha wrote:
>> From: Mark Wielaard <mark@klomp.org>
>>
>> On the current AArch64 buildbot glibc-fedora-arm64
>> the 30s timeout is not enough to run this test, the
>> machine has 8 cores, but the testcase is deliberately
>> single threaded
>>
>>   make test t=locale/tst-localedef-path-norm
>>
>> takes about 45s, so I increased the timeout to 60s.
>>
>> This is the only failure on this buildbot builder.
>> https://builder.sourceware.org/buildbot/#/builders/glibc-fedora-arm64
>> ---
>>  locale/tst-localedef-path-norm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/locale/tst-localedef-path-norm.c b/locale/tst-localedef-path-norm.c
>> index 68995a415..80e1c556d 100644
>> --- a/locale/tst-localedef-path-norm.c
>> +++ b/locale/tst-localedef-path-norm.c
>> @@ -236,5 +236,5 @@ do_test (void)
>>    return 0;
>>  }
>>  
>> -#define TIMEOUT 30
>> +#define TIMEOUT 60

A default timeout of 60s is too long. You need to increase TIMEOUTFACTOR if you
have a slow disk attached to the system under test.

>>  #include <support/test-driver.c>
> 
> I think it would be maybe better to either split the test, so we can
> improve make check parallel runtime; or use a large TIMEOUTFACTOR.

We could. There is a balance here because the tests need root and write to / to
exercise the path normalization code. My comment from 2020 explains this balance.

102   /* It takes ~10 seconds to serially execute 9 localedef test.  We
103      could run the compilations in parallel if we want to reduce test
104      time.  We don't want to split this out into distinct tests because
105      it would require multiple chroots.  Batching the same localedef
106      tests saves disk space during testing.  */
Mark Wielaard July 19, 2022, 2:20 p.m. UTC | #3 
Hi Adhemerval,

On Mon, Jul 18, 2022 at 04:50:45PM -0300, Adhemerval Zanella Netto wrote:
> > On the current AArch64 buildbot glibc-fedora-arm64
> > the 30s timeout is not enough to run this test, the
> > machine has 8 cores, but the testcase is deliberately
> > single threaded
> > 
> >   make test t=locale/tst-localedef-path-norm
> > 
> > takes about 45s, so I increased the timeout to 60s.
> > 
> > This is the only failure on this buildbot builder.
> > https://builder.sourceware.org/buildbot/#/builders/glibc-fedora-arm64
> > ---
> >  locale/tst-localedef-path-norm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/locale/tst-localedef-path-norm.c b/locale/tst-localedef-path-norm.c
> > index 68995a415..80e1c556d 100644
> > --- a/locale/tst-localedef-path-norm.c
> > +++ b/locale/tst-localedef-path-norm.c
> > @@ -236,5 +236,5 @@ do_test (void)
> >    return 0;
> >  }
> >  
> > -#define TIMEOUT 30
> > +#define TIMEOUT 60
> >  #include <support/test-driver.c>
> 
> I think it would be maybe better to either split the test, so we can
> improve make check parallel runtime;

That would be nice, but the test was explicitly written to not be
split up to not require multiple chroots and save disk space.

> or use a large TIMEOUTFACTOR.

Yeah, I thought about that, but this is the only testcase that takes
such a long time. It seems better to make sure that all tests work out
of the box with a proper timeout than let people struggle with
figuring out the right TIMEOUTFACTOR just because one test runs a
little longer (unless they really have a slow board to
test where multiple tests might time out on of course).
 
> As a side note, we also added some string tests that require a lot
> time so it would be good to reevaluate them for next release.

I did noticed there are various string tests with timeouts of multiple
minutes. But it looks like those do run in parallel. What was somewhat
surprising is that make check -j8 is not much faster than make check
-j4. But that seems to be mostly because the nptl tests are all run
serially.

Cheers,

Mark
Adhemerval Zanella July 19, 2022, 2:34 p.m. UTC | #4 
On 19/07/22 11:20, Mark Wielaard wrote:
> Hi Adhemerval,
> 
> On Mon, Jul 18, 2022 at 04:50:45PM -0300, Adhemerval Zanella Netto wrote:
>>> On the current AArch64 buildbot glibc-fedora-arm64
>>> the 30s timeout is not enough to run this test, the
>>> machine has 8 cores, but the testcase is deliberately
>>> single threaded
>>>
>>>   make test t=locale/tst-localedef-path-norm
>>>
>>> takes about 45s, so I increased the timeout to 60s.
>>>
>>> This is the only failure on this buildbot builder.
>>> https://builder.sourceware.org/buildbot/#/builders/glibc-fedora-arm64
>>> ---
>>>  locale/tst-localedef-path-norm.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/locale/tst-localedef-path-norm.c b/locale/tst-localedef-path-norm.c
>>> index 68995a415..80e1c556d 100644
>>> --- a/locale/tst-localedef-path-norm.c
>>> +++ b/locale/tst-localedef-path-norm.c
>>> @@ -236,5 +236,5 @@ do_test (void)
>>>    return 0;
>>>  }
>>>  
>>> -#define TIMEOUT 30
>>> +#define TIMEOUT 60
>>>  #include <support/test-driver.c>
>>
>> I think it would be maybe better to either split the test, so we can
>> improve make check parallel runtime;
> 
> That would be nice, but the test was explicitly written to not be
> split up to not require multiple chroots and save disk space.

I think we can still parallelize the tests by running each test on
different thread [1].  Each tests write on independent file, so it
should be ok to issue localedef in parallel.

[1] https://patchwork.sourceware.org/project/glibc/patch/20220719140113.1604672-1-adhemerval.zanella@linaro.org/

> 
>> or use a large TIMEOUTFACTOR.
> 
> Yeah, I thought about that, but this is the only testcase that takes
> such a long time. It seems better to make sure that all tests work out
> of the box with a proper timeout than let people struggle with
> figuring out the right TIMEOUTFACTOR just because one test runs a
> little longer (unless they really have a slow board to
> test where multiple tests might time out on of course).

I tend to agree, I would prefer if we can fix this specific test.

>  
>> As a side note, we also added some string tests that require a lot
>> time so it would be good to reevaluate them for next release.
> 
> I did noticed there are various string tests with timeouts of multiple
> minutes. But it looks like those do run in parallel. What was somewhat
> surprising is that make check -j8 is not much faster than make check
> -j4. But that seems to be mostly because the nptl tests are all run
> serially.
> 

Yes, we have added them to check for corner cases and they tests multiple
alignments with multiple page configuration.  I think we should split
them in next release, some tests take about 40s even on a really fast
processor (Ryzen9 5900).
diff mbox series

Patch

diff --git a/locale/tst-localedef-path-norm.c b/locale/tst-localedef-path-norm.c
index 68995a415..80e1c556d 100644
--- a/locale/tst-localedef-path-norm.c
+++ b/locale/tst-localedef-path-norm.c
@@ -236,5 +236,5 @@  do_test (void)
   return 0;
 }
 
-#define TIMEOUT 30
+#define TIMEOUT 60
 #include <support/test-driver.c>