Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test
- From
Victoria Dye <vdye@github.com>
- Date
- Sep 8, 2022, 17:29 UTC
- Message-ID
- <6bc8f50b-c869-11a7-b12e-dca8a2ce2a81@github.com>
- In-Reply-To
- <220908.86edwml3hm.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason wrote:
Show 43 quoted lines
>
> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> As suggested in
>> https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
>> t7112 can run for well over one hour, which seems to be the default
>> maximum run time at least when running CTest-based tests in Visual
>> Studio.
>>
>> Let's increase the time-out as a stop gap to unblock developers wishing
>> to run Git's test suite in Visual Studio.
>>
>> Note: The actual run time is highly dependent on the circumstances. For
>> example, in Git's CI runs, the Windows-based tests typically take a bit
>> over 5 minutes to run. CI runs have the added benefit that Windows
>> Defender (the common anti-malware scanner on Windows) is turned off,
>> something many developers are not at liberty to do on their work
>> stations. When Defender is turned on, even on this developer's high-end
>> Ryzen system, t7112 takes over 15 minutes to run.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> contrib/buildsystems/CMakeLists.txt | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
>> index 29d7e236ae1..b1306f95256 100644
>> --- a/contrib/buildsystems/CMakeLists.txt
>> +++ b/contrib/buildsystems/CMakeLists.txt
>> @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
>> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>> endforeach()
>>
>> +# This test script takes an extremely long time and is known to time out even
>> +# on fast machines because it requires in excess of one hour to run
>> +set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
>> +
>> endif()#BUILD_TESTING
>
> I don't see per [1] that it would have any negative effect to just bump
> the timeout a lot more, and for all the tests.I'm a bit confused as to how that link (which explains that the 'TIMEOUT' property is used to determine when to kill a test process - more or less standard behavior for timeouts in applications) shows that excessively long timeouts on every test "[wouldn't] have any negative effect."
In terms of the timeout length, the stated reason for adding the timeout in the first place is:
>> Let's increase the time-out as a stop gap to unblock developers wishing >> to run Git's test suite in Visual Studio.
I would consider ~1 hour a manageable (albeit frustrating) amount of time to wait for a test, but not 10 hours (as you suggest later).
As for setting the timeout on all tests: that might be helpful, only because (AFAICT from CMake's source code & documentation) the default timeout is "no timeout." If setting a global default, though, I'd prefer a shorter timespan (e.g. 1800s) so that a timeout properly indicates a failure condition ("this test runs much longer than it should, so we may have introduced a performance bug").
Practically, though, I don't feel too strongly about whether or not to set a global default. If we wanted to add timeouts to more tests in the future, I'd want a more extensible solution with carefully-selected timeouts. But if it's just this one test running too long, special case-ing a timeout for it with a comment explaining the reasoning seems appropriate to me.
Show 6 quoted lines
> > If we're running into 3600 seconds, then setting this to 4000 seconds > seems to be a smal stopgap at best. That's just over a 10% increase, so > if one person ran into it it 3600 someone with a slightly slower system > should be running into the same still, or if we just add a few more > tests to 7112 (or some other slow test).
I don't think this patch is suggesting that timeout failures are happening at 3600s, rather that the peak observed execution time of the test is 3600s. If that's the case, <peak execution time> + an 11% buffer is a reasonable choice for the value.
Show 5 quoted lines
> > So why not set this ta 3600*10 or whatever for *all* of the test scripts > instead of playing whack-a-mole? > > 1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html