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.

Sign up for GitHub

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

Simplify OpenQA::WebAPI::Controller::Test #4968

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Simplify OpenQA::WebAPI::Controller::Test #4968

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9430da
Use signatures in OpenQA::WebAPI::Controller::Test
okurz Jun 15, 2021
fa1bc3b
Simplify OpenQA::WebAPI::Controller::Test
okurz Jun 15, 2021
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
72 changes: 20 additions & 52 deletions lib/OpenQA/WebAPI/Controller/Test.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,16 @@ use constant DEPENDENCY_DEBUG_INFO => $ENV{OPENQA_DEPENDENCY_DEBUG_INFO};

sub referer_check ($self) {
return $self->reply->not_found if (!defined $self->param('testid'));
my $referer = $self->req->headers->header('Referer') // '';
return 1 unless $referer;
return undef unless my $referer = $self->req->headers->header('Referer') // '';
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it would always have returned 1. Maybe this is somehow causing the tests to fail?

$self->schema->resultset('Jobs')->mark_job_linked($self->param('testid'), $referer);
return 1;
}

sub list {
my ($self) = @_;
}

sub get_match_param {
my ($self) = @_;
sub list ($self) { }

my $match;
if (defined($self->param('match'))) {
$match = $self->param('match');
$match =~ s/[^\w\[\]\{\}\(\),:.+*?\\\$^|-]//g; # sanitize
}
return $match;
sub get_match_param ($self) {
return unless my $match = $self->param('match');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return unless my $match = $self->param('match');
return undef unless my $match = $self->param('match');

We should always return something. If I'm reading it correctly undef is what we want here.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, and looking at the usage this is very important here because we use the method in list context

Copy link
Contributor

Choose a reason for hiding this comment

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

This would now also return early if the parameter contains 0.

$match =~ s/[^\w\[\]\{\}\(\),:.+*?\\\$^|-]//gr; # sanitize
}

sub list_ajax ($self) {
Expand Down Expand Up @@ -124,9 +115,7 @@ sub _render_comment_data_for_ajax ($self, $job_id, $comment_data) {
return \%data;
}

sub list_running_ajax {
my ($self) = @_;

sub list_running_ajax ($self) {
my $running = $self->schema->resultset('Jobs')->complex_query(
state => [OpenQA::Jobs::Constants::EXECUTION_STATES],
match => $self->get_match_param,
Expand Down Expand Up @@ -166,8 +155,7 @@ sub list_running_ajax {
$self->render(json => {data => \@running});
}

sub list_scheduled_ajax {
my ($self) = @_;
sub list_scheduled_ajax ($self) {
my $limits = OpenQA::App->singleton->config->{misc_limits};
my $limit = min($limits->{generic_max_limit}, $self->param('limit') // $limits->{generic_default_limit});

Expand Down Expand Up @@ -218,9 +206,7 @@ sub _stash_job ($self, $args = undef) {
return $job;
}

sub _stash_job_and_module_list {
my ($self, $args) = @_;

sub _stash_job_and_module_list ($self, $args) {
return undef unless my $job = $self->_stash_job($args);
my $test_modules = read_test_modules($job);
$self->stash(modlist => ($test_modules ? $test_modules->{modules} : []));
Expand Down Expand Up @@ -298,9 +284,7 @@ sub downloads ($self) {
$self->render('test/downloads');
}

sub settings {
my ($self) = @_;

sub settings ($self) {
$self->_stash_job({prefetch => 'settings'}) or return $self->reply->not_found;
$self->render('test/settings');
}
Expand All @@ -316,8 +300,7 @@ So this works in the same way as the test module source.

=cut

sub show_filesrc {
my ($self) = @_;
sub show_filesrc ($self) {
my $job = $self->_stash_job or return $self->reply->not_found;
my $jobid = $self->param('testid');
my $dir = $self->stash('dir');
Expand Down Expand Up @@ -363,9 +346,7 @@ sub show_filesrc {
);
}

sub comments {
my ($self) = @_;

sub comments ($self) {
$self->_stash_job({prefetch => 'comments', order_by => 'comments.id'}) or return $self->reply->not_found;
$self->render('test/comments');
}
Expand All @@ -380,8 +361,7 @@ sub _stash_clone_info ($self, $job) {
});
}

sub infopanel {
my ($self) = @_;
sub infopanel ($self) {
my $job = $self->_stash_job or return $self->reply->not_found;
$self->stash({worker => $job->assigned_worker, additional_data => 1});
$self->_stash_clone_info($job);
Expand All @@ -398,8 +378,7 @@ sub _get_current_job ($self, $with_assets = 0) {

sub show ($self) { $self->_show($self->_get_current_job(1)) }

sub _show {
my ($self, $job) = @_;
sub _show ($self, $job) {
return $self->reply->not_found unless $job;

$self->stash(
Expand Down Expand Up @@ -473,9 +452,7 @@ sub job_next_previous_ajax ($self) {
$self->render(json => {data => \@data});
}

sub _calculate_preferred_machines {
my ($jobs) = @_;

sub _calculate_preferred_machines ($jobs) {
my %machines;
for my $job (@$jobs) {
next unless my $machine = $job->MACHINE;
Expand Down Expand Up @@ -616,9 +593,7 @@ sub _prepare_job_results ($self, $all_jobs, $limit) {
}

# appends the specified $distri and $version to $array_to_add_parts_to as string or if $raw as Mojo::ByteStream
sub _add_distri_and_version_to_summary {
my ($array_to_add_parts_to, $distri, $version, $raw) = @_;

sub _add_distri_and_version_to_summary ($array_to_add_parts_to, $distri, $version, $raw) {
for my $part ($distri, $version) {
# handle case when multiple distri/version parameters have been specified
$part = $part->{-in} if (ref $part eq 'HASH');
Expand All @@ -641,8 +616,7 @@ sub _add_distri_and_version_to_summary {
}

# A generic query page showing test results in a configurable matrix
sub overview {
my ($self) = @_;
sub overview ($self) {
my ($search_args, $groups) = $self->compose_job_overview_search_args;
my $config = OpenQA::App->singleton->config;
my $validation = $self->validation;
Expand Down Expand Up @@ -704,8 +678,7 @@ sub overview {
html => {template => 'test/overview'});
}

sub latest {
my ($self) = @_;
sub latest ($self) {
my %search_args = (limit => 1);
for my $arg (OpenQA::Schema::Result::Jobs::SCENARIO_WITH_MACHINE_KEYS) {
my $key = lc $arg;
Expand All @@ -718,9 +691,7 @@ sub latest {
return $self->_show($job);
}

sub module_fails {
my ($self) = @_;

sub module_fails ($self) {
return $self->reply->not_found unless defined $self->param('testid') and defined $self->param('moduleid');
my $module = $self->app->schema->resultset("JobModules")->search(
{
Expand All @@ -741,7 +712,7 @@ sub module_fails {
}

# Fallback to first step
$first_failed_step = 1 if $first_failed_step == 0;
$first_failed_step ||= 1;

$self->render(
json => {
Expand Down Expand Up @@ -803,7 +774,6 @@ sub _add_dependency_to_node ($node, $parent, $dependency_type) {
}

sub _add_job ($dependency_data, $job, $as_child_of, $preferred_depth) {

# add current job; return if already visited
my $job_id = $job->id;
my $visited = $dependency_data->{visited};
Expand Down Expand Up @@ -857,7 +827,6 @@ sub _add_job ($dependency_data, $job, $as_child_of, $preferred_depth) {
}

sub dependencies ($self) {

# build dependency graph starting from the current job
my $job = $self->_get_current_job or return $self->reply->not_found;
my (@nodes, @edges, %cluster);
Expand All @@ -866,8 +835,7 @@ sub dependencies ($self) {
$self->render(json => {nodes => \@nodes, edges => \@edges, cluster => \%cluster});
}

sub investigate {
my ($self) = @_;
sub investigate ($self) {
return $self->reply->not_found unless my $job = $self->_get_current_job;
my $git_limit = OpenQA::App->singleton->config->{global}->{job_investigate_git_log_limit} // 200;
my $investigation = $job->investigate(git_limit => $git_limit);
Expand Down