Merge pull request #6005 from Martchus/worker-coverage

Improve test coverage of worker code
os-autoinst · Oct 15, 2024 · 726e974 · 726e974
2 parents cd422ea + c2c9fe6
commit 726e974
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 31 deletions.
diff --git a/codecov.yml b/codecov.yml
@@ -12,6 +12,10 @@ coverage:
     project:
       default:
         target: 98
+      fully_covered:
+        target: 100.0
+        paths:
+          - lib/openQA/Worker
       tests:
         target: 100.0
         paths:

diff --git a/lib/OpenQA/Worker/CommandHandler.pm b/lib/OpenQA/Worker/CommandHandler.pm
@@ -76,14 +76,12 @@ sub handle_command {
                 return undef;
             }
         }
-        else {
+        elsif (defined $job_id) {
             # ignore messages which belong to a job
-            if (defined $job_id) {
-                log_warning("Ignoring WS message from $webui_host with type $type and job ID $job_id "
-                      . "(currently not executing a job):\n"
-                      . pp($json));
-                return undef;
-            }
+            log_warning("Ignoring WS message from $webui_host with type $type and job ID $job_id "
+                  . "(currently not executing a job):\n"
+                  . pp($json));
+            return undef;
         }
         # verify that the web UI host for the job ID matches as well
         if ($job_id && $webui_host ne $current_webui_host) {

diff --git a/lib/OpenQA/Worker/Engines/isotovideo.pm b/lib/OpenQA/Worker/Engines/isotovideo.pm
@@ -368,9 +368,8 @@ sub _configure_cgroupv2 ($job_info) {
     my $cgroup;
     eval {
         $cgroup = cgroupv2(name => $cgroup_name)->from($cgroup_slice)->child($job_info->{id})->create;
-        if (my $query_cgroup_path = $cgroup->can('_cgroup')) {
-            log_info('Using cgroup ' . $query_cgroup_path->($cgroup));
-        }
+        my $query_cgroup_path = $cgroup->can('_cgroup');
+        log_info('Using cgroup ' . $query_cgroup_path->($cgroup)) if $query_cgroup_path;
     };
     if (my $error = $@) {
         $cgroup = c();
@@ -484,7 +483,7 @@ sub _engine_workit_step_2 ($job, $job_settings, $vars, $shared_cache, $callback)
             # Allow to override isotovideo executable with an arbitrary
             # command line based on a config option
             exec $job_settings->{ISOTOVIDEO} ? $job_settings->{ISOTOVIDEO} : ('perl', $isotovideo, '-d');
-            die "exec failed: $!\n";
+            die "exec failed: $!\n";    # uncoverable statement
         });
     $child->on(
         collected => sub {

diff --git a/lib/OpenQA/Worker/WebUIConnection.pm b/lib/OpenQA/Worker/WebUIConnection.pm
@@ -140,6 +140,7 @@ sub _setup_websocket_connection ($self, $websocket_url = undef) {
                 disabled => {
                     error_message => "Unable to establish ws connection to $webui_host without worker ID"
                 });
+            return undef;
         }
         $websocket_url = $self->url->clone;
         my %ws_scheme = (http => 'ws', https => 'wss');

diff --git a/t/24-worker-engine.t b/t/24-worker-engine.t
@@ -316,7 +316,13 @@ subtest '_handle_asset_processed' => sub {
     $status->data->{has_download_error} = 1;    # assume download error
     $error = OpenQA::Worker::Engines::isotovideo::_handle_asset_processed(@args);
     is ref $error, 'HASH', 'error when download failed although asset is still existing'
-      and $error->{error}, 'Failed to download path2 to some/path2', 'expected error message returned';
+      and is $error->{error}, 'Failed to download path2 to some/path2', 'expected error message returned';
+
+    $status = OpenQA::CacheService::Response::Status->new(error => 'error');
+    $args[1] = 'UEFI_PFLASH_VARS';
+    delete $vars->{UEFI_PFLASH_VARS};
+    is OpenQA::Worker::Engines::isotovideo::_handle_asset_processed(@args), undef, 'no error for UEFI_PFLASH_VARS';
+    is $vars->{UEFI_PFLASH_VARS}, undef, 'asset URI not set because asset does not exist';
 };
 
 subtest 'syncing tests' => sub {
@@ -532,4 +538,11 @@ subtest 'link asset' => sub {
     is $vars_data->{HDD_1}, 'foo.qcow2', 'the value of HDD_1 is basename when doing link';
 };
 
+subtest 'using cgroupv2' => sub {
+    my $file_mock = Test::MockModule->new('Mojo::File');
+    $file_mock->noop('make_path');
+    combined_like { OpenQA::Worker::Engines::isotovideo::_configure_cgroupv2({id => 42}) }
+    qr|Using cgroup /sys/fs/cgroup/.*/42|, 'use of cgroup logged';
+};
+
 done_testing();
diff --git a/t/24-worker-webui-connection.t b/t/24-worker-webui-connection.t
@@ -83,9 +83,27 @@ $job_mock->redefine(start_livelog => sub { shift->{_livelog_viewers} = 1 });
 $job_mock->redefine(stop_livelog => sub { shift->{_livelog_viewers} = 0 });
 
 subtest 'attempt to register and send a command' => sub {
-    # test registration failure
-    $client->register;
-    is($client->status, 'failed', 'client failed to register');
+    my @expected_events;
+    subtest 'handling registration failure on connection error' => sub {
+        $client->register;
+        is $client->status, 'failed', 'client failed to register after connection error';
+        push @expected_events, {status => 'registering', error_message => undef}, {status => 'failed'};
+    };
+    subtest 'handling registration failure due to validation error' => sub {
+        my $ua_mock = Test::MockModule->new('Mojo::UserAgent');
+        my $fake_tx = Mojo::Transaction::HTTP->new;
+        $fake_tx->res->code(200);
+        $fake_tx->res->headers->content_type('text/json');
+        $fake_tx->res->body('{}');
+        $ua_mock->redefine(post => $fake_tx);
+        $client->register;
+        is $client->status, 'disabled', 'client failed to register after validation error';
+        push @expected_events, {status => 'registering', error_message => undef},
+          {
+            error_message => 'Failed to register at http://test-host: host did not return a worker ID',
+            status => 'disabled'
+          };
+    };
 
     # note: Successful registration is tested in e.g. `05-scheduler-full.t`.
 
@@ -149,23 +167,30 @@ subtest 'attempt to register and send a command' => sub {
     is($client->worker->stop_current_job_called,
         WORKER_SR_API_FAILURE, 'attempted to stop current job with reason "api-failure"');
 
-    my $error_message = ref($happened_events[1]) eq 'HASH' ? delete $happened_events[1]->{error_message} : undef;
-    (
-        is_deeply(
-            \@happened_events,
-            [{status => 'registering', error_message => undef}, {status => 'failed'}],
-            'events emitted',
-          )
-          and like($error_message, qr{Failed to register at http://test-host - connection error:.*}, 'error message')
-    ) or diag explain \@happened_events;
+    subtest 'emitted events' => sub {
+        my $error_message = ref($happened_events[1]) eq 'HASH' ? delete $happened_events[1]->{error_message} : undef;
+        is_deeply \@happened_events, \@expected_events, 'expected events emitted';
+        like $error_message, qr{Failed to register at http://test-host - connection error:.*}, 'error message';
+    } or diag explain \@happened_events;
 };
 
 subtest 'attempt to setup websocket connection' => sub {
     my @expected_events = (
+        {
+            status => 'disabled',
+            error_message => 'Unable to establish ws connection to http://test-host without worker ID'
+        },
         {status => 'establishing_ws', error_message => undef},
         {status => 'failed', error_message => 'Unable to upgrade to ws connection via http://test-host/api/v1/ws/42'},
     );
     @happened_events = ();
+
+    # attempt to connect without worker ID
+    $client->worker_id(undef);
+    $client->_setup_websocket_connection;
+
+    # attempt to connect running into connection error
+    $client->worker_id(42);
     $client->_setup_websocket_connection;
     $client->once(status_changed => sub ($status, @) { Mojo::IOLoop->stop if $status eq 'failed' });
     Mojo::IOLoop->start;
@@ -492,10 +517,8 @@ qr/Ignoring WS message from http:\/\/test-host with type livelog_stop and job ID
     is($accepted_job->developer_session_running, 1, 'developer session running');
     $command_handler->handle_command(undef, {type => WORKER_COMMAND_LIVELOG_STOP, jobid => 25});
     is($accepted_job->livelog_viewers, 0, 'livelog stopped');
-
-    combined_like { $command_handler->handle_command(undef, {type => WORKER_COMMAND_QUIT, jobid => 27}) }
-    qr/Ignoring job cancel from http:\/\/test-host because there's no job with ID 27/,
-      'ignoring commands for different job';
+    combined_like { $command_handler->handle_command(undef, {type => 'livelog_stop', jobid => 21}) }
+    qr/Ignoring WS message.*for job 21.*not running/, 'livelog command for other job ignored';
 
     # stopping job
     is($accepted_job->status, 'new',
@@ -530,11 +553,29 @@ qr/Ignoring WS message from http:\/\/test-host with type livelog_stop and job ID
 
     # test incompatible (so far the worker stops when receiving this message; there are likely better ways to handle it)
     is($worker->is_stopping, 0, 'not already stopping');
-    combined_like {
-        $command_handler->handle_command(undef, {type => 'incompatible'})
-    }
+    combined_like { $command_handler->handle_command(undef, {type => 'incompatible'}) }
     qr/running a version incompatible with web UI host http:\/\/test-host and therefore stopped/, 'problem is logged';
     is($worker->is_stopping, 1, 'worker is stopping on incompatible message');
+
+    $client->webui_host('foo');
+    $worker->current_webui_host('bar');
+    $worker->current_job(OpenQA::Worker::Job->new($worker, $client, {id => 42}));
+    combined_like { $command_handler->handle_command(undef, {type => 'quit'}) }
+    qr/Ignoring job cancel from foo.*currently working for bar/, 'stop command from other web UI host ignored';
+    combined_like { $command_handler->handle_command(undef, {type => 'livelog_stop', jobid => 42}) }
+    qr/Ignoring job-specific WS message.*foo.*currently occupied by bar/, 'live command from other web UI host ignored';
+
+    $worker->current_webui_host('foo');
+    combined_like { $command_handler->handle_command(undef, {type => 'quit'}) }
+    qr/Ignoring job cancel from foo.*no job ID/, 'stop command without job ID ignored';
+
+    combined_like { $command_handler->handle_command(undef, {type => 'quit', jobid => 21}) }
+    qr/Ignoring job cancel from foo.*no job with ID 21/, 'stop command for unknown job ignored';
+
+    $worker->pending_job(OpenQA::Worker::Job->new($worker, $client, {id => 43}));
+    combined_like { $command_handler->handle_command(undef, {type => 'quit', jobid => 43}) }
+    qr/Will quit job 43 later as requested by the web UI/, 'stop command for pending job executed later';
+    is_deeply $worker->skipped_jobs, [[43, 'quit']], 'pending job is going to be skipped';
 };
 
 $client->worker_id(undef);

diff --git a/t/lib/OpenQA/Test/FakeWorker.pm b/t/lib/OpenQA/Test/FakeWorker.pm
@@ -15,8 +15,10 @@ has current_webui_host => undef;
 has capabilities => sub { {fake_capabilities => 1} };
 has stop_current_job_called => 0;
 has is_stopping => 0;
+has skipped_jobs => sub { [] };
 has current_error => undef;
 has current_job => undef;
+has pending_job => undef;
 has has_pending_jobs => 0;
 has pending_job_ids => sub { [] };
 has current_job_ids => sub { [] };
@@ -34,9 +36,12 @@ sub accept_job ($self, $client, $job_info) {
 sub enqueue_jobs_and_accept_first ($self, $client, $job_info) {
     $self->enqueued_job_info($job_info);
 }
+sub skip_job ($self, $job_id, $type) { push @{$self->skipped_jobs}, [$job_id, $type] }
 sub find_current_or_pending_job ($self, $job_id) {
     return undef unless my $current_job = $self->current_job;
     return $current_job if $current_job->id eq $job_id;
+    return undef unless my $pending_job = $self->pending_job;
+    return $pending_job if $pending_job->id eq $job_id;
 }
 
 1;