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

Introduce check_shutdown for testapi #988

Merged
merged 2 commits into from Aug 21, 2018

Conversation

OleksandrOrlov
Copy link

The check_shutdown function is added to have ability to decide on further actions in case of shutdown failure, instead of marking a test as failed immediately.

In tests it might be used to make soft failure checks, for example.

Related ticket: poo#35215

@codecov
Copy link

codecov bot commented Jul 6, 2018

Codecov Report

Merging #988 into master will decrease coverage by 11.54%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #988       +/-   ##
===========================================
- Coverage   37.55%   26.01%   -11.55%     
===========================================
  Files          34       34               
  Lines        4489     4494        +5     
  Branches      764      765        +1     
===========================================
- Hits         1686     1169      -517     
- Misses       2486     3126      +640     
+ Partials      317      199      -118
Impacted Files Coverage Δ
testapi.pm 6.5% <0%> (+1.13%) ⬆️
OpenQA/Benchmark/Stopwatch.pm 5% <0%> (-65%) ⬇️
ppmclibs/blib/lib/tinycv.pm 5.43% <0%> (-41.31%) ⬇️
consoles/VNC.pm 3.89% <0%> (-38.75%) ⬇️
consoles/vnc_base.pm 9.52% <0%> (-33.34%) ⬇️
backend/baseclass.pm 20.2% <0%> (-27.95%) ⬇️
consoles/console.pm 27.5% <0%> (-25%) ⬇️
OpenQA/Qemu/ControllerConf.pm 35% <0%> (-20%) ⬇️
OpenQA/Qemu/SnapshotConf.pm 10.25% <0%> (-17.95%) ⬇️
OpenQA/Qemu/DriveDevice.pm 52.94% <0%> (-13.73%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea37ecd...59789e7. Read the comment docs.

@OleksandrOrlov OleksandrOrlov changed the title Introduce check_shutdown for testapi [WIP] Introduce check_shutdown for testapi Jul 6, 2018
Copy link
Member

@foursixnine foursixnine left a comment

Choose a reason for hiding this comment

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

Overall looks good, but I think adding unit tests for it woudn't hurt


=cut

sub assert_shutdown {
sub check_shutdown {
Copy link
Member

Choose a reason for hiding this comment

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

You will need to use Test::MockModule, mock the query_isotovideo method from autotest to return the expected values. An example on how to do that can be seen in the testapi tests


Returns C<undef> on success, throws exception on timeout.
Returns true on success and false if C<$timeout> timeout is hit. Default timeout is 60s.
Copy link
Member

Choose a reason for hiding this comment

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

An update to the pod wouldn't hurt :)

@Martchus
Copy link
Contributor

Martchus commented Jul 17, 2018

Looks good now that you've added tests.

This changes the meaning of assert_shutdown which is a breaking API change. So I suppose $INTERFACE should be incremented (https://github.com/os-autoinst/os-autoinst/blob/master/isotovideo#L66)?

Copy link
Member

@foursixnine foursixnine left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far, except for that -1

is(assert_shutdown, undef, 'assert_shutdown should return "undef" if shutdown finished before timeout is hit');
$mod->mock(
read_json => sub {
return {ret => -1};
Copy link
Member

Choose a reason for hiding this comment

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

why -1 specifically?

Copy link
Author

Choose a reason for hiding this comment

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

It is because I just wanted to cover the following part in check_shutdown function:

if ($is_shutdown < 0) {
    bmwqemu::diag("Backend does not implement is_shutdown - just sleeping");
    sleep($timeout);
}

The check_shutdown function is added
to have ability to decide on further
actions in case of shutdown failure,
instead of marking a test as failed
immediately.

In tests it might be used to make
soft failure checks.

Related ticket: [poo#35215](https://progress.opensuse.org/issues/35215)
@OleksandrOrlov OleksandrOrlov changed the title [WIP] Introduce check_shutdown for testapi Introduce check_shutdown for testapi Jul 18, 2018
@@ -9,6 +9,7 @@ use Test::Fatal;
use Test::Mock::Time;
use Test::Warnings;
use File::Temp;
use Test::Exception;
Copy link
Member

Choose a reason for hiding this comment

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

Please add this dependency to the cpanfile.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@OleksandrOrlov OleksandrOrlov force-pushed the 35215_check_shutdown branch 5 times, most recently from 0c3f540 to d130cdc Compare August 6, 2018 19:51
@Martchus
Copy link
Contributor

@foursixnine If the things you've mentioned are fixed now, we should merge this.

@foursixnine foursixnine merged commit 5f7b73c into os-autoinst:master Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants