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

Show previous results in test results page #538

Merged
merged 4 commits into from Feb 17, 2016

Conversation

Projects
None yet
5 participants
@okurz
Copy link
Member

commented Feb 10, 2016

Querying the database for former test runs of the same scenario is a rather
costly operation which we do not want to do for multiple test results at once
but only for each individual test result (1:1 relation).

Related issue: https://progress.opensuse.org/issues/10212

Screenshot of feature:
screenshot_20160210_142024

  • test results: Add failure details to previous jobs
  • Also use direct reference to db in "sub show"
  • test results: Add tab with "previous results" in same scenario
  • templates: test: Reload selected tab also when no modlist

Tested against production database dump from openqa.opensuse.org, querying with

time sudo -u geekotest OPENQA_CONFIG=local/stage OPENQA_DATABASE=stage script/openqa get '/tests/117629' >/dev/null 2>&1 | grep real

yields the following time increase before and after change

  • origin/master: 0.8s
  • feature/label_and_previous_results: 1.1s

@okurz okurz force-pushed the okurz:feature/label_and_previous_results branch 2 times, most recently from 3a79826 to c0e258d Feb 10, 2016

@@ -293,6 +293,33 @@ sub _job_get($) {
return $job->to_hash(assets => 1);
}

sub job_settings_subquery {

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

I guess the memo was sent before you joined - but we declared Scheduler.pm as dead. Code should be removed from it, not added. So please put this code into the model.

my @conds;
push(@conds, {'me.state' => 'done'});
push(@conds, {'me.result' => {-not_in => [OpenQA::Schema::Result::Jobs::INCOMPLETE_RESULTS]}});
# instead of searching for "previous jobs" it might be better to search for same or previous builds

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

"previous build" is unfortunately in reality very unreliable, so I think you don't really have an alternative to what you do

@@ -233,7 +233,7 @@ sub read_test_modules {
}

sub show {
my ($self) = @_;
my ($self, $limit_previous) = @_;

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

there are no arguments to mojo controllers. So if you want to declare an undefined variable, just do so :)

@coolo

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

please also rebase, so the diff is a bit smaller

@@ -0,0 +1,7 @@
% if ($comments and $comments > 0) {

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

you're cheating :)

This comment has been minimized.

Copy link
@okurz

okurz Feb 11, 2016

Author Member

I am sorry, I don't want to. I commented on this in the corresponding commit message:
okurz@c0e258d

The 'if ($comment and $comments > 0)' is used to prevent warnings about
undefined if comments are not available at all but also not show notifications
if it is always defined by 0 as used in 'previous.html.ep'.

Don't know a better way except for needing to count the available comments within Jobs.pm#show like in sub overview.

This comment has been minimized.

Copy link
@aaannz

aaannz Feb 11, 2016

Member

Ah, there are $comment and $comments. Just spent 5min staring at this :) . What about if ($comment and defined $comments)?

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

the code comment talks about $comment , the code does not - it's $comments only

This comment has been minimized.

Copy link
@aaannz

aaannz Feb 11, 2016

Member

Ah, right. So what is the issue with plain if ($comments) again? Am I missing something?

This comment has been minimized.

Copy link
@okurz

okurz Feb 11, 2016

Author Member

this code section is included in two locations, templates/test/previous.html.ep with the resultset including comments as parameter (always defined but can be empty) and templates/test/tr_job_result.html.ep with $res->{comments} as parameter which is a scalar (undef if no comments).
If I just check with if ($comments) then it will always be true for "previous.html" and always display the symbol. If I just check with if ($comments > 0) then it will yield a warning for "tr_job_result.html".

So what approach do you prefer?

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

previous sets $comments to the number of comments, right? if ($comments) will be false if that number is 0. So where is the problem?

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

[ that's the difference between if ($comments) and if (defined $comments) ]

This comment has been minimized.

Copy link
@aaannz

aaannz Feb 11, 2016

Member

As @coolo said. L26 clearly set scalar as $comments.

push(@conds, {'me.id' => {-in => $subquery->get_column('job_id')->as_query}});

my %attrs = (
rows => $limit_previous //= 10, # arbitrary limit of previous results to show

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

please move that assignment in a line of its own. this is very ugly IMO

my $previous_results_header = $t->tx->res->dom->at('#previous #scenario')->all_text;
is($previous_results_header, q/Results for opensuse-13.1-DVD-i586-textmode, limited to 10/, 'header for previous results with scenario');
$get->element_exists('#res_99945', 'result from previous job');
$get->element_exists('#res_99945 .result_passed', 'previous job was passed');

This comment has been minimized.

Copy link
@coolo

coolo Feb 11, 2016

Contributor

@sysrich completely offtopic: what's the signficance of was here? 'it passed' is past, 'it has passed' is perfect. What is 'was passed'? It's a lot in the internet, so it must be correct - but what is it? :)

This comment has been minimized.

Copy link
@okurz

okurz Feb 15, 2016

Author Member

You can read this as either "past perfect" to describe an action that completed before another event after that which is also already in the past or you can read it as "the job was completed with the result of 'passed'" which is what I intended here.

This comment has been minimized.

Copy link
@coolo

coolo Feb 17, 2016

Contributor

but past perfect is built with had. So it's either "the job had passed" as past perfect or "the job was passed" as past of be with the adjective 'passed'. So much about the grammar. But neither past perfect makes sense here as there is no other event - nor does past, as the job's state is passed even in the future. It won't change anymore. Anyway, I was just curious.

@coolo

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

BTW: I would be curious how this behaves on a production copy data

@okurz

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

BTW: I would be curious how this behaves on a production copy data

yes, plan for today

@okurz okurz force-pushed the okurz:feature/label_and_previous_results branch 2 times, most recently from 73c7d55 to 101653a Feb 15, 2016

okurz added some commits Feb 10, 2016

test results: Add tab with "previous results" in same scenario
Querying the database for former test runs of the same scenario is a rather
costly operation which we do not want to do for multiple test results at once
but only for each individual test result (1:1 relation).

Related issue: https://progress.opensuse.org/issues/10212
test results: Add failure details to previous jobs
Extract helper templates from overview template and reuse them in failure
details on previous jobs in test results page.

@okurz okurz force-pushed the okurz:feature/label_and_previous_results branch from 101653a to a7c1228 Feb 16, 2016

@okurz

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2016

updated:

  • performance test with production database dump (see description)
  • rebased (now also showing labels on previous results)
  • javascript adjustment on results page: Reload selected tab also when no modlist

updated screenshot:
openqa_previous_results_with_labels

@coolo, @sysrich ping

coolo added a commit that referenced this pull request Feb 17, 2016

Merge pull request #538 from okurz/feature/label_and_previous_results
Show previous results in test results page

@coolo coolo merged commit 8da815f into os-autoinst:master Feb 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Soulofdestiny

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2016

If a test with the same name ran on different machines (e.g. "allpatterns" and "allpatterns@zkvm", it founds multiple previous jobs
intentional or bug?

screenshot from 2016-03-01 15-43-36

@sysrich

This comment has been minimized.

Copy link
Member

commented Mar 1, 2016

I think it's a bug, but it's an awesome one ;)

@okurz please change the filter so the previous jobs filter based on machine used by the job, not arch :)

@okurz okurz deleted the okurz:feature/label_and_previous_results branch Mar 1, 2016

@okurz

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.