Limit test execution time in tests #3024
Conversation
This comment has been minimized.
This comment has been minimized.
okurz
changed the title
okurz
force-pushed
the
feature/limit_test_time2
branch
from
07f2f9c
to
cfff7e2
Compare
okurz
changed the title
|
changed to simply use |
Codecov Report
@@ Coverage Diff @@
## master #3024 +/- ##
==========================================
- Coverage 92.01% 92.00% -0.02%
==========================================
Files 211 211
Lines 12877 12877
==========================================
- Hits 11849 11847 -2
- Misses 1028 1030 +2
Continue to review full report at Codecov.
|
okurz
force-pushed
the
feature/limit_test_time2
branch
from
cfff7e2
to
67b1799
Compare
okurz
changed the title
|
Somehow the subject line prefix "[WIP][RFC]" wasn't inviting enough for you to leave your feedback so I am now trying differently :) I consider this ready to be merged and used if one of you can approve :) |
There was a problem hiding this comment.
When things are executed asynchronously we already have certain timeouts when waiting for somethings. And the problem is that it is that sometimes the CI system is just really slow (seems to hang for multiple seconds). Hence these tests are sometimes failing needlessly. I guess we will run into the same problem here as well.
| my ($package, $limit) = @_; | ||
| die "$package: Need argument on import, e.g. use: use OpenQA::Test::TimeLimit '42';" unless $limit; | ||
| $SIG{ALRM} = sub { die "test exceeds runtime limit of '$limit' seconds\n" }; | ||
| alarm $limit; |
There was a problem hiding this comment.
It is usually a mistake to intermix alarm and sleep calls, because sleep may be internally implemented on your system with alarm.
(see https://perldoc.perl.org/functions/alarm.html)
Our test code is full with sleeps. So it is likely a bad idea to use this module blindly. I'm also wondering about interference with e.g. Mojo::IOLoop.
There was a problem hiding this comment.
Yes, I am aware and so far I have not encountered problems in both basic tests as well as more complicated ones, e.g. also t/full-stack.t that uses sleep and spawns multiple subprocesses.
There was a problem hiding this comment.
I suppose using alarm here is not harmful but it might just not work when it is overridden somewhere during the test execution.
t/basic.t
Outdated
| @@ -20,6 +20,7 @@ use lib "$FindBin::Bin/lib"; | |||
| use Test::Mojo; | |||
| use Test::Warnings; | |||
| use OpenQA::Test::Database; | |||
| use OpenQA::Test::TimeLimit '6'; | |||
There was a problem hiding this comment.
I'm wondering about this weird usage. A simple test helper, e.g. called set_timeout didn't work?
There was a problem hiding this comment.
That would be boring :) As discussed in person I consider it a usual treat of perl modules to provide functionality on import and also in a configurable way. This is inspired by https://metacpan.org/pod/Time::Limit . Additional advantages I see with this approach:
- The limit is already effective from the time of import. Otherwise one would need to mix function calls with imports which is not that nice
- Saving some lines as the only functionality of this module is the module-wide time limit so there would be no need for further lines of code anyway
There was a problem hiding this comment.
"That would be boring" / "Saving some lines" - those are bad justifications. But "The limit is already effective from the time of import." is enough for me so why not. I think that explanation should go into a comment in t/lib/OpenQA/Test/TimeLimit.pm.
okurz
force-pushed
the
feature/limit_test_time2
branch
from
67b1799
to
6d51134
Compare
okurz
force-pushed
the
feature/limit_test_time2
branch
from
6d51134
to
d8776db
Compare
| @@ -20,6 +20,7 @@ use Test::Warnings; | |||
| use FindBin; | |||
| use lib "$FindBin::Bin/lib"; | |||
| use OpenQA::Test::Database; | |||
| use OpenQA::Test::TimeLimit '10'; | |||
There was a problem hiding this comment.
I reckon this or a variant of it with a higher timeout should be added to OpenQA::Test::Case? Otherwise it means we need to add it to virtual all tests by hand.
I'm just guessing, though. The PR has no description.
There was a problem hiding this comment.
Added a description.
I suggest we gather experiences with explicit mentions first. If we like the idea we can extend the generic test base class. We only need to be a bit careful as it is less explicit. However I do not see a problem with defining a conservative, rather high limit. Also when that limit is below the limit within our CI system I think the feedback is even more explicit as it comes from our tests and not the external CI harness.
There was a problem hiding this comment.
Ack. Maybe different defaults for e.g. UI tests also makes sense. Just the idea of having loads of different settings per file made me a bit uncomfortable at first...
Tests are only fun to call when they are not taking too long hence it makes sense to check their runtime against unintended runtime increases as happened some times in the past. Also this helps to save time for persons executing tests encountering other problems that lead to tests getting stuck as the timeout would abort the test early.
This PR is adding a test module that can be included in – at best – all tests setting a reasonable test-specific timeout to prevent an unintented increase of runtime. Also added is the example use of the test module in a single test with the idea to use the test module in more and more cases.