Extend iso post TEST parameter to support a list of tests (poo#10482)

[cwh42]

I hope you actually use 'poo' as a short form of 'progress.opensuse.org' ;)

[aaannz]

Missing test. ;)

[coveralls]

Coverage decreased (-0.05%) to 64.454% when pulling 18fe1a1 on cwh42:upstream into 03edbc2 on os-autoinst:master.

[cwh42]

Indeed. I guess I'll get to that next week.
The test should prove that the right test suites are scheduled. Not sure if there already is any test that checks that.

[coolo]

any update?

[cwh42]

Trying that on in t/api/02-iso.t failed:
# handle list of tests
$ret = $t->post_ok('/api/v1/isos', form => {iso => $iso, DISTRI => 'opensuse',
VERSION => '13.1', FLAVOR => 'DVD', ARCH => 'i586',
test => "textmode,kde,client1"})->status_is(200);

Any hints?

[aaannz]

Start the test as MOJO_LOG_LEVEL=debug perl t/api/02-iso.t and see what the error is on the openQA side.

[cwh42]

@aaannz that helped, thanks. I even start to extend my test. Stay tuned.

[cwh42]

I know the test fails, since it schedules 4 tests. I wonder if this can be right.

[okurz]

please delete disabled code

[cwh42]

@aaannz I did it the same way as here: https://github.com/cwh42/openQA/blob/upstream/t/api/02-iso.t#L120

[aaannz]

Yeah, but that is parsing result from /api/v1/isos call. Here you are parsing /api/v1/jobs

[aaannz]

I'm not against standardizing these and to prevent breakage you would need to add count to /api/v1/jobs call.

[cwh42]

While cleaning up after running 'tidy' I managed to mix up those two lines.
Updated.

[okurz]

just for checking the number of jobs you don't need the get call but just check the return value of the post but the problem at hand is that 4 jobs are scheduled when 3 are expected.

[cwh42]

And all 4 match the desired three TEST names.

[okurz]

my change proposal

$ git log -p HEAD^^..
commit cd7898faae8cff4db72a72a88296983d3d04c55c
Author: Oliver Kurz <okurz@suse.de>
Date:   Tue May 10 11:15:48 2016 +0200

    fixup! Test for "let TEST parameter support a list of tests"

diff --git a/t/api/02-iso.t b/t/api/02-iso.t
index 657f3db..0863907 100644
--- a/t/api/02-iso.t
+++ b/t/api/02-iso.t
@@ -186,7 +186,7 @@ is($ret->tx->res->json->{job}->{state}, 'cancelled', "job $newid is cancelled");
 $ret = $t->post_ok('/api/v1/isos', form => {iso => $iso, tests => "kde/usb"})->status_is(400);

 # handle list of tests
-my $iso2 = 'openSUSE-13.1-DVD-i586-Build0093-Media.iso';
+my $iso2 = 'openSUSE-13.1-DVD-i586-Build0091-Media.iso';
 $ret = $t->post_ok(
     '/api/v1/isos',
     form => {
@@ -196,14 +196,10 @@ $ret = $t->post_ok(
         FLAVOR  => 'DVD',
         ARCH    => 'i586',
         TEST    => 'server,kde,textmode',
-        BUILD   => '0093'
+        BUILD   => '0091'
     })->status_is(200);

-$ret = $t->get_ok('/api/v1/jobs');
-is($ret->tx->res->json->{count}, 3, "3 new jobs created");
-
-#print "Job Count: ".$ret->tx->res->json->{count}."\n"; # DEBUG
-#print Dumper($ret->tx->res->json->{jobs}); # DEBUG
+is($ret->tx->res->json->{count}, 4, "4 new jobs created (one twice for both machine types)");

 # delete the iso
 # can not do as operator

commit 672f7ad36e34b82b2ee988fd1ec4c58d826984d7
Author: Oliver Kurz <okurz@suse.de>
Date:   Tue May 10 11:15:27 2016 +0200

    t: Also print machine tag for unambiguous job lists

diff --git a/t/api/02-iso.t b/t/api/02-iso.t
index 0977b68..657f3db 100644
--- a/t/api/02-iso.t
+++ b/t/api/02-iso.t
@@ -48,7 +48,7 @@ sub lj {
     my $ret  = $t->get_ok('/api/v1/jobs')->status_is(200);
     my @jobs = @{$ret->tx->res->json->{jobs}};
     for my $j (@jobs) {
-        printf "%d %-10s %s\n", $j->{id}, $j->{state}, $j->{name};
+        printf "%d %-10s %s@%s\n", $j->{id}, $j->{state}, $j->{name}, $j->{settings}->{MACHINE};
     }
 }

[cwh42]

@okurz Patch applied. Test succeeds (at least if run locally). Thanks!

[aaannz]

I think this can be removed, or at least not changed.

[cwh42]

To be consequent then, I suggest to also remove other unneeded 'use's:

diff --git a/t/api/02-iso.t b/t/api/02-iso.t
index 0863907..2a0bac6 100644
--- a/t/api/02-iso.t
+++ b/t/api/02-iso.t
@@ -19,15 +19,12 @@ BEGIN {
unshift @inc, 'lib';
}

-use Mojo::Base -strict;
use Test::More;
use Test::Mojo;
use Test::Warnings ':all';
-use Test::Output qw/stdout_like stderr_like/;
use OpenQA::Test::Case;
use OpenQA::Client;
use Mojo::IOLoop;
-use Data::Dumper;

use OpenQA::IPC;
use OpenQA::WebSockets;

[okurz]

yes, in general. Your commit should just change what is necessary, i.e. don't change 'use Data::Dump' as you don't touch this as @aaannz suggested. I think the use Mojo::Base -strict should be kept. It is like use strict; The others could be removed if unused now but in another commit.

[aaannz]

Now that test is written, I would like to optimize a little. In current implementation you (1) parse $args{TEST} every iteration of test templates and machines. And (2) you iterate through all values of TEST and continue even when match is found. There is no need to do that.
So I propose for (1) to move my @tests = split(/\s*,\s*/, $args->{TEST}); somewhere up before entering first for cycles (L126 looks fine) and for (2) add last; after L196.

[cwh42]

Changed test and implementation according @aaannz & @okurz suggestions. Any more comments?

[okurz]

+1, need to fix test, though

[aaannz]

Undef warning in case of $args->{TEST} is not present.

[cwh42]

Looks better now.

[coveralls]

Coverage increased (+0.07%) to 65.113% when pulling eecc12c on cwh42:upstream into b6cd6a6 on os-autoinst:master.

[aaannz]

👍