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

Carry over labels from previous jobs in same scenario if still failing #564

Closed
wants to merge 4 commits into from
Closed

Carry over labels from previous jobs in same scenario if still failing #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0986432
Extract 'bugref' and 'label' helpers into methods on comment
okurz Feb 19, 2016
192155a
Carry over labels from previous jobs in same scenario if still failing
okurz Feb 23, 2016
40303a6
Do not carry over labels over passes
okurz Feb 23, 2016
e110f86
t: Add more tests for labels_carry_over for a complete coverage
okurz Feb 23, 2016
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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
COVERAGE_THRESHOLD ?= 59.8
COVERAGE_THRESHOLD ?= 60.1

.PHONY: all
all:
Expand Down
44 changes: 44 additions & 0 deletions lib/OpenQA/Scheduler/Scheduler.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,48 @@ sub _job_stop_children {
}
}

=head2 _carry_over_labels

carry over labels (i.e. special comments) from previous jobs to current result
in the same scenario.

=cut
sub _carry_over_labels {
my ($job) = @_;

# search for previous jobs
my @scenario_keys = qw/DISTRI VERSION FLAVOR ARCH TEST/;
my %js_settings = map { $_ => $job->settings_hash->{$_} } @scenario_keys;
# arbitrary limit not to search all jobs
my $limit_previous = 200;
my $subquery = schema->resultset("JobSettings")->query_for_settings(\%js_settings);
my @conds = [{'me.state' => 'done'}, {id => {'<', $job->id}}, {'me.id' => {-in => $subquery->get_column('job_id')->as_query}}];
my %attrs = (
rows => $limit_previous,
order_by => ['me.id DESC']);
my $previous_jobs = schema->resultset("Jobs")->search({-and => \@conds}, \%attrs);

# loop through all comments on all previous jobs in same scenario
# if comment is label OR bug carry over comment to current_job
while (my $prev = $previous_jobs->next) {
last if ($prev->result eq 'passed'); # do not carry over labels over passes
my $comments = schema->resultset('Comments')->search({job_id => $prev->id}, {order_by => {-desc => 'me.id'}});
while (my $comment = $comments->next) {
if ($comment->bugref or $comment->label) {
my $rs = $job->comments->create(
{
text => $comment->text,
# TODO can we also use another user id to tell that
# this comment was created automatically and not by a
# human user?
user_id => $comment->user_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using original comment creator is ok, or create something like comment link. For user-less events in audit log I use undef as NULL user id, but that to work you would need to change Schema/Result/Comments:L53 to

__PACKAGE__->belongs_to(owner => 'OpenQA::Schema::Result::Users', 'user_id', {join_type => 'left'});

Copy link
Member Author

Choose a reason for hiding this comment

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

I am thinking about extending this further down the road to keep a bit of history, e.g. encode the original author but create the new comment by "the machine". So let's stick the the original author for now and extend this later.

});
return;
}
}
}
}

=head2 job_set_done

mark job as done. No error check. Meant to be called from worker!
Expand Down Expand Up @@ -707,6 +749,8 @@ sub job_set_done {
if ($result ne OpenQA::Schema::Result::Jobs::PASSED) {
_job_skip_children($jobid);
_job_stop_children($jobid);
# labels are there to mark reasons of failure
_carry_over_labels($job);
}
return $r;
}
Expand Down
21 changes: 21 additions & 0 deletions lib/OpenQA/Schema/Result/Comments.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,27 @@ __PACKAGE__->belongs_to(
},
);


=head2 bugref

Returns bugref if C<$self> is bugref, e.g. 'bug#1234'.
=cut
sub bugref {
my ($self) = @_;
$self->text =~ /\b([^t]+#\d+)\b/;
return $1;
}

=head2 label

Returns label value if C<$self> is label, e.g. 'label:my_label' returns 'my_label'
=cut
sub label {
my ($self) = @_;
$self->text =~ /\blabel:(\w+)\b/;
return $1;
}

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

Expand Down
12 changes: 6 additions & 6 deletions lib/OpenQA/WebAPI/Controller/Test.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,13 +348,13 @@ sub _job_labels {
# behaviour for multiple bugs or label references within one job is
# undefined.
while (my $comment = $c->next()) {
if ($comment->text =~ /\b[^t]+#\d+\b/) {
$self->app->log->debug('found bug ticket reference ' . $& . ' for job ' . $comment->job_id);
$labels{$comment->job_id}{bug} = $&;
if (my $bugref = $comment->bugref) {
$self->app->log->debug('found bug ticket reference ' . $bugref . ' for job ' . $comment->job_id);
$labels{$comment->job_id}{bug} = $bugref;
}
elsif ($comment->text =~ /\blabel:(\w+)\b/) {
$self->app->log->debug('found label ' . $1 . ' for job ' . $comment->job_id);
$labels{$comment->job_id}{label} = $1;
elsif (my $label = $comment->label) {
$self->app->log->debug('found label ' . $label . ' for job ' . $comment->job_id);
$labels{$comment->job_id}{label} = $label;
}
else {
$labels{$comment->job_id}{comments}++;
Expand Down
110 changes: 110 additions & 0 deletions t/17-labels_carry_over.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
# Copyright (C) 2016 SUSE Linux GmbH
Copy link
Contributor

Choose a reason for hiding this comment

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

Its SUSE LLC now.

Copy link
Member Author

Choose a reason for hiding this comment

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

are you sure:

$ git grep -h Copyright | sort | uniq --count | sort -n | tail -n 10
      2 # Copyright (C) 2016 Red Hat
      2 # Copyright (C) 2016 SUSE Linux GmbH
      3 # Copyright (C) 2015 Red Hat
      3 # Copyright (C) 2016 SUSE LLC
      6 # Copyright (c) 2015 SUSE LINUX GmbH, Nuernberg, Germany.
      8  * Copyright jQuery Foundation and other contributors
     10 # Copyright (C) 2015 SUSE LLC
     11 # Copyright (C) 2015 SUSE Linux Products GmbH
     35 # Copyright (C) 2015 SUSE Linux GmbH
     92 # Copyright (C) 2014 SUSE Linux Products GmbH

;-P

I would not mind a copyright checker script, until then …

Copy link
Member

Choose a reason for hiding this comment

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

SUSE Linux GmbH and SUSE LLC are both valid

SUSE Linux Products GmbH is no longer valid
On 24 Feb 2016 7:18 pm, "Oliver Kurz" notifications@github.com wrote:

In t/17-labels_carry_over.t
#564 (comment):

@@ -0,0 +1,110 @@
+# Copyright (C) 2016 SUSE Linux GmbH

are you sure:

$ git grep -h Copyright | sort | uniq --count | sort -n | tail -n 10
2 # Copyright (C) 2016 Red Hat
2 # Copyright (C) 2016 SUSE Linux GmbH
3 # Copyright (C) 2015 Red Hat
3 # Copyright (C) 2016 SUSE LLC
6 # Copyright (c) 2015 SUSE LINUX GmbH, Nuernberg, Germany.
8 * Copyright jQuery Foundation and other contributors
10 # Copyright (C) 2015 SUSE LLC
11 # Copyright (C) 2015 SUSE Linux Products GmbH
35 # Copyright (C) 2015 SUSE Linux GmbH
92 # Copyright (C) 2014 SUSE Linux Products GmbH

;-P

I would not mind a copyright checker script, until then …

—
Reply to this email directly or view it on GitHub
https://github.com/os-autoinst/openQA/pull/564/files#r53981489.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, @lnussel initiated discussion with legal about that and response was:

SUSE LLC is the owner of SUSE IP. You should use it everywhere. But
don't delete but rather append.

Then there was this #441 (see outdated diff) and since then we started replacing all to SUSE LLC. Not all at once, of course, only when file is changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

SUSE LLC is the one we should use for new code. SUSE Linux GmbH only exists to pay our bills

Copy link
Member Author

Choose a reason for hiding this comment

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

we agreed @aaannz will take a look into checking this automatically.

@coolo

  1. wanna keep as is and merge
  2. I change this single line with a very cost-inefficient commit fixup when other files are still wrong
  3. you have more suggestions how to improve, e.g. regarding the code in Scheduler
    ?

#
# 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, write to the Free Software Foundation, Inc.,
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

BEGIN {
unshift @INC, 'lib';
}

use Mojo::Base -strict;
use Test::More;
use Test::Mojo;
use OpenQA::Test::Case;
use OpenQA::IPC;
use OpenQA::Scheduler;
use OpenQA::WebSockets;
use JSON qw/decode_json/;

my $ipc = OpenQA::IPC->ipc('', 1);
my $sh = OpenQA::Scheduler->new;
my $ws = OpenQA::WebSockets->new;

my $test_case;
my $t;
my $auth;

sub set_up {
$test_case = OpenQA::Test::Case->new;
$test_case->init_data;
$t = Test::Mojo->new('OpenQA::WebAPI');
$auth = {'X-CSRF-Token' => $t->ua->get('/tests')->res->dom->at('meta[name=csrf-token]')->attr('content')};
$test_case->login($t, 'percival');
}

sub comments {
my ($url) = @_;
my $get = $t->get_ok($url)->status_is(200);
return $get->tx->res->dom->find('div.comments .media-comment ~ p')->map('content');
}

sub restart_with_result {
my ($old_job, $result) = @_;
my $get = $t->post_ok("/api/v1/jobs/$old_job/restart", $auth)->status_is(200);
my $res = decode_json($get->tx->res->body);
my $new_job = $res->{result}[0];
$t->post_ok("/api/v1/jobs/$new_job/set_done", $auth => form => {result => $result})->status_is(200);
return $res;
}
set_up;

subtest '"happy path": failed->failed carries over last label' => sub {
my $label = 'label:false_positive';
my $second_label = 'bsc#1234';
my $simple_comment = 'just another simple comment';
for my $comment ($label, $second_label, $simple_comment) {
$t->post_ok('/tests/99962/add_comment', $auth => form => {text => $comment})->status_is(302);
}
my @comments_previous = @{comments('/tests/99962')};
is(scalar @comments_previous, 3, 'all entered comments found');
is($comments_previous[0], $label, 'comment present on previous test result');
is($comments_previous[2], $simple_comment, 'another comment present');
$t->post_ok('/api/v1/jobs/99963/set_done', $auth => form => {result => 'failed'})->status_is(200);
my @comments_current = @{comments('/tests/99963')};
is(scalar @comments_current, 1, 'only one label is carried over');
like($comments_current[0], qr/\Q$second_label/, 'last entered label found, it is expanded');
};

my $job;
subtest 'failed->passed discards the label' => sub {
my $res = restart_with_result(99963, 'passed');
$job = $res->{result}[0];
my @comments_new = @{comments($res->{test_url}[0])};
is(scalar @comments_new, 0, 'no labels carried over to passed');
};

subtest 'passed->failed does not carry over old label' => sub {
my $res = restart_with_result($job, 'failed');
$job = $res->{result}[0];
my @comments_new = @{comments($res->{test_url}[0])};
is(scalar @comments_new, 0, 'no old labels on new failure');
};

subtest 'failed->failed without labels does not fail' => sub {
my $res = restart_with_result($job, 'failed');
$job = $res->{result}[0];
my @comments_new = @{comments($res->{test_url}[0])};
is(scalar @comments_new, 0, 'nothing there, nothing appears');
};

subtest 'failed->failed labels which are not bugrefs are also carried over' => sub {
my $label = 'label:any_label';
$t->post_ok("/tests/$job/add_comment", $auth => form => {text => $label})->status_is(302);
my $res = restart_with_result($job, 'failed');
my @comments_new = @{comments($res->{test_url}[0])};
is(scalar @comments_new, 1, 'also simple labels are carried over');
is($comments_new[0], $label, 'simple label present in new result');
};

done_testing;
2 changes: 1 addition & 1 deletion templates/test/previous.html.ep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
<td class="t_finished">
<abbr class="timeago" title="<%= $prev->t_finished->datetime() %>Z"><%= format_time($prev->t_finished) %></abbr>
(
<%= format_time_duration($prev->t_finished - $prev->t_started) %>
<%= $job->t_started ? format_time_duration($prev->t_finished - $prev->t_started) : 0 %>
)
</td>
</tr>
Expand Down
2 changes: 1 addition & 1 deletion templates/test/result.html.ep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
finished
<abbr class="timeago" title="<%= $job->t_finished->datetime() %>Z"><%= format_time($job->t_finished) %></abbr>
(
<%= format_time_duration($job->t_finished - $job->t_started) %>
<%= $job->t_started ? format_time_duration($job->t_finished - $job->t_started) : 0 %>
)
% } elsif ($job->t_started)
% {
Expand Down