Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit test execution time in tests #3024

Merged
merged 2 commits into from
May 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions t/basic.t
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use Test::Warnings;
use FindBin;
use lib "$FindBin::Bin/lib";
use OpenQA::Test::Database;
use OpenQA::Test::TimeLimit '10';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


OpenQA::Test::Database->new->create(skip_fixtures => 1);
Test::Mojo->new('OpenQA::WebAPI')->get_ok('/')->status_is(200)->content_like(qr/Welcome to openQA/i);
Expand Down
72 changes: 72 additions & 0 deletions t/lib/OpenQA/Test/TimeLimit.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Copyright (C) 2020 SUSE LLC
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License along
# with this program; if not, see <http://www.gnu.org/licenses/>.

package OpenQA::Test::TimeLimit;
use strict;
use warnings;

sub import {
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;
Copy link
Contributor

@Martchus Martchus May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose using alarm here is not harmful but it might just not work when it is overridden somewhere during the test execution.

}

1;

=encoding utf8

=head1 NAME

OpenQA::Test::TimeLimit - Limit test runtime

=head1 SYNOPSIS

use OpenQA::Test::TimeLimit '42';

=head1 DESCRIPTION

This aborts a test if the specified runtime limit in seconds is
exceeded.

Example output for t/basic.t:

t/basic.t .. run... failed: test exceeds runtime limit of '1' seconds

Example output for t/full-stack.t:

ok 1 - assets are prefetched
[info] [pid:4324] setting database search path to public when registering Minion plugin
[info] Listening at "http://127.0.0.1:35182"
Server available at http://127.0.0.1:35182
Bailout called. Further testing stopped: get: Server returned error message test exceeds runtime limit of '6' seconds at /home/okurz/local/os-autoinst/openQA/t/lib/OpenQA/SeleniumTest.pm:107
Bail out! get: Server returned error message test exceeds runtime limit of '6' seconds at /home/okurz/local/os-autoinst/openQA/t/lib/OpenQA/SeleniumTest.pm:107
FAILED--Further testing stopped: get: Server returned error message test exceeds runtime limit of '6' seconds at /home/okurz/local/os-autoinst/openQA/t/lib/OpenQA/SeleniumTest.pm:107

=head2 Alternatives considered

* Just checking the runtime while not aborting the test – this idea has
not been followed as we want to prevent any external runners to run into
timeout first which can cause less obvious results
* https://metacpan.org/pod/Time::Limit - nice syntax that inspired me to
use a parameter on import but fails to completely stop tests including
all subprocesses
* https://metacpan.org/pod/Time::Out - applies a timeout to blocks, not
a complete module
* https://metacpan.org/pod/Time::SoFar - easy and simple but not
providing enough value to include
* https://metacpan.org/pod/Acme::Time::Baby - Just kidding ;)

=cut