Merge lp://staging/~nuclearbob/qa-dashboard/categories into lp://staging/qa-dashboard

FAILED: Continuous integration, rev:707
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nuclearbob/qa-dashboard/categories/+merge/199552/+edit-commit-message

http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/282/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/282/rebuild

 review needs-fixing

The first thing I noticed is no tests. :-(

The second thing I noticed, @properties are bad.. :-) This MP took
the 'image_overview.html'
page from 5 queries to 136 queries and the 'test_results.html' page from 10
queries to 16 queries... This is going to need an overhaul before it can be
merged in.

According to the doc:

   1.

   When looking at an individual test failure, expert level knowledge is
   required in order to determine whether the test failure is due to:
   1.

      An application regression
      2.

      A poorly written test.
      3.

      A bug in autopilot.
      4. A problem with the CI infrastructure.

Currently it appears as though the ability to triage failures is open to
anyone as long as they login. Based on the doc, that doesn't seem like a
desired effect. It seems like the ability to triage should be limited to
certain teams.

i'm in a hurry and didn't look too closely, but I think you could improve your categories methods by skipping the for-loops and using either django annotate or aggregate (i forget which). I haven't looked holistically at your code, but you might see if that helps

> review needs-fixing
>
> The first thing I noticed is no tests. :-(
>
I started on those, but I wanted to get implementation feedback before finishing them so I didn't write a lot of tests for code I ended up discarding. I'll get those in as I'm addressing the other issues.
> The second thing I noticed, @properties are bad.. :-) This MP took
> the 'image_overview.html'
> page from 5 queries to 136 queries and the 'test_results.html' page from 10
> queries to 16 queries... This is going to need an overhaul before it can be
> merged in.
>
Is there a tool or option you're using to track this that I can use to test fixes?
>
> According to the doc:
>
>
> 1.
>
> When looking at an individual test failure, expert level knowledge is
> required in order to determine whether the test failure is due to:
> 1.
>
> An application regression
> 2.
>
> A poorly written test.
> 3.
>
> A bug in autopilot.
> 4. A problem with the CI infrastructure.
>
>
> Currently it appears as though the ability to triage failures is open to
> anyone as long as they login. Based on the doc, that doesn't seem like a
> desired effect. It seems like the ability to triage should be limited to
> certain teams.
That makes sense to me. I'll discuss it with the team and figure out how we want to manage what team(s) have that access.

Also, Andy, thanks for mentioning those functions, I'll take a look.

On Thu, Jan 2, 2014 at 9:08 AM, Max Brustkern
<email address hidden>wrote:

> > The second thing I noticed, @properties are bad.. :-) This MP took
> > the 'image_overview.html'
> > page from 5 queries to 136 queries and the 'test_results.html' page from
> 10
> > queries to 16 queries... This is going to need an overhaul before it can
> be
> > merged in.
> >
> Is there a tool or option you're using to track this that I can use to
> test fixes?
>

django-debug-toolbar

> review needs-fixing
>
> The first thing I noticed is no tests. :-(
I think I have the implementation in better shape now, so I'll re-propose the merge with tests once they're ready.
>
> The second thing I noticed, @properties are bad.. :-)
Should I just remove the decorator and have these as functions?
> This MP took the 'image_overview.html'
> page from 5 queries to 136 queries and the 'test_results.html' page from 10
> queries to 16 queries... This is going to need an overhaul before it can be
> merged in.
Is one additional query on image_overview.html and test_results.html okay? I've got both of those to that point now.
>
>
> According to the doc:
>
>
> 1.
>
> When looking at an individual test failure, expert level knowledge is
> required in order to determine whether the test failure is due to:
> 1.
>
> An application regression
> 2.
>
> A poorly written test.
> 3.
>
> A bug in autopilot.
> 4. A problem with the CI infrastructure.
>
>
> Currently it appears as though the ability to triage failures is open to
> anyone as long as they login. Based on the doc, that doesn't seem like a
> desired effect. It seems like the ability to triage should be limited to
> certain teams.
I discussed this with my team. It looks like the dashboard doesn't currently request launchpad teams in the OAUTH request, so that would have to be added. Do we foresee this being enough of a problem to adjust the existing authentication routines, or would it be sufficient for now to log categorizations being made and add additional constraints if improper ones are found?

On Tue, Jan 07, 2014 at 04:29:23PM -0000, Max Brustkern wrote:
> > review needs-fixing
> >
> > The first thing I noticed is no tests. :-(
> I think I have the implementation in better shape now, so I'll re-propose the merge with tests once they're ready.
> >
> > The second thing I noticed, @properties are bad.. :-)
> Should I just remove the decorator and have these as functions?

@properties aren't bad in general only those that access the db. It
perfectly acceptable to do something like this.

@property
def display_name(self):
    return "{} {}".format(self.first_name, self.last_name)

> On Tue, Jan 07, 2014 at 04:29:23PM -0000, Max Brustkern wrote:
> > > review needs-fixing
> > >
> > > The first thing I noticed is no tests. :-(
> > I think I have the implementation in better shape now, so I'll re-propose
> the merge with tests once they're ready.
> > >
> > > The second thing I noticed, @properties are bad.. :-)
> > Should I just remove the decorator and have these as functions?
>
> @properties aren't bad in general only those that access the db. It
> perfectly acceptable to do something like this.
>
> @property
> def display_name(self):
> return "{} {}".format(self.first_name, self.last_name)

Okay, I figured since SmokeImage.summary was a property, I was working on something similar to that. I can make those back into functions.

I've taken some steps to address the concerns raised. I'd appreciate additional feedback on what else might be required, as well as thoughts on the issue of authentication vs. logging.

Evan would like to add the support so that only members of certain teams can perform these actions.

Okay. I'll learn how to work with OpenID and come back once that's ready. Any resources you'd suggest?

everything you will need should be in django-openid-auth and the docs are
pretty good.

On Tue, Jan 14, 2014 at 9:40 AM, Max Brustkern
<email address hidden>wrote:

> Okay. I'll learn how to work with OpenID and come back once that's ready.
> Any resources you'd suggest?
> --
>
> https://code.launchpad.net/~nuclearbob/qa-dashboard/categories/+merge/199552
> You are reviewing the proposed merge of
> lp:~nuclearbob/qa-dashboard/categories into lp:qa-dashboard.
>

--
Chris Johnston <email address hidden>
Software Engineer - CI Engineering Team
Canonical Ltd.
www.ubuntu.com

Cool, thanks.

It looks like to use launchpad teams, we need to use launchpad as our openid provider instead of ubuntu SSO. That's an easy change to make, but is it going to create a problem for users that already have accounts?

I was wrong about using launchpad instead of ubuntu for SSO. The new changes add an authentication group mapped to the CI, QA, and bug control teams. This group is required for categorization.

PASSED: Continuous integration, rev:720
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/286/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/286/rebuild

On Thu, Jan 16, 2014 at 06:06:29PM -0000, Max Brustkern wrote:
> Max Brustkern has proposed merging lp:~nuclearbob/qa-dashboard/categories into lp:qa-dashboard.
>
> Commit message:
> Added categorization for automated test failures
>
> Requested reviews:
> Max Brustkern (nuclearbob)
> Chris Johnston (cjohnston)
> PS Jenkins bot (ps-jenkins): continuous-integration
> Joe Talbott (joetalbott)
>
> For more details, see:
> https://code.launchpad.net/~nuclearbob/qa-dashboard/categories/+merge/199552
>
> This branch adds the ability to categorize test failures at the test result detail level. It displays aggregate views of test failures at the test results and image overview levels.
> --
> https://code.launchpad.net/~nuclearbob/qa-dashboard/categories/+merge/199552
> You are requested to review the proposed merge of lp:~nuclearbob/qa-dashboard/categories into lp:qa-dashboard.

> === added file 'common/migrations/0011_add_auth_group.py'
> --- common/migrations/0011_add_auth_group.py 1970-01-01 00:00:00 +0000
> +++ common/migrations/0011_add_auth_group.py 2014-01-16 18:04:46 +0000
> @@ -0,0 +1,25 @@
> +# -*- coding: utf-8 -*-
> +import datetime
> +from south.db import db
> +from south.v2 import DataMigration
> +from django.db import models
> +from django.contrib.auth.models import Group
> +
> +class Migration(DataMigration):
> +
> + def forwards(self, orm):
> + Group.objects.create(name='image-test-triage')

I think this needs to be orm['django.contrib.auth.Group'].objects... or
similar.

It might be different since this is a django model and not an app model,
but I know there's usually a warning about it in auto-generated
migrations.

Joe

I'll look into that further. It was migrating with what I had before, but doing it the right way is definitely better. So far I'm mostly getting things like:
KeyError: "The model 'contrib.auth.group' from the app 'django' is not available in this migration."
when I try to use orm.

Okay, that should be taken care of now.

PASSED: Continuous integration, rev:721
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/287/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/287/rebuild

On Thu, Jan 16, 2014 at 06:06:29PM -0000, Max Brustkern wrote:
> === modified file 'smokeng/views.py'
> --- smokeng/views.py 2013-11-21 16:58:17 +0000
> +++ smokeng/views.py 2014-01-16 18:04:46 +0000
> @@ -17,6 +17,7 @@
> import re
>
> from django.db import models
> +from django.forms.models import modelform_factory
> from django.shortcuts import (
> render_to_response,
> get_object_or_404,
> @@ -298,6 +299,7 @@
> 'summary': summary,
> 'build_number': image.build_number,
> 'release': release,
> + 'categories': image.categories(),
> 'table': table,
> 'bread_crumb_trail': BreadCrumbTrail.leading_to(
> image_overview,
> @@ -417,6 +419,7 @@
> 'totals': totals,
> 'jenkins_url': jenkins_url,
> 'private_url': private_url,
> + 'categories': result.categories(),
> 'table': table,
> 'artifacts': artifacts,
> 'history': _test_history(result),
> @@ -468,7 +471,6 @@
> 'id',
> ],
> )
> -@require_GET

I think this should be:

@require_http_methods(["GET", "POST"])

Make sure to import it along with require_GET.

Other than that I think it looks fine.

Joe

That should be fixed now.

PASSED: Continuous integration, rev:722
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/288/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/288/rebuild

Can we please add, at a minimum testing that the permissions works as well as testing of submitting the form.

Sure. Any hints on how I should write those tests? Should I be looking at the actual page that gets rendered for the permission one, or is there a better way to do it? Is there an easier django-riffic way to test forms, or do I just need to fire up a server and send a POST to it and then check the db?

http://toastdriven.com/blog/2011/apr/10/guide-to-testing-in-django/
http://toastdriven.com/blog/2011/apr/17/guide-to-testing-in-django-2/

Awesome, thanks!

I've added some additional tests.

PASSED: Continuous integration, rev:726
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/289/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/289/rebuild

One big issue I have with the current implementation is that it appears as
though every test is an 'uncategorized failure'... If you look at
http://ubuntuone.com/1LimdwSyCIPp6qH0CXZ3J9 it shows 93 uncategorized
failures, but there are no failing tests or tests with errors..

PASSED: Continuous integration, rev:727
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/291/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/291/rebuild

FAILED: Continuous integration, rev:728
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/292/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/292/rebuild

PASSED: Continuous integration, rev:731
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/293/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/293/rebuild

I've reworked the way the box displays per a discussion with cjohnston, and updated tests for that. The discrepancy in the count of categorized results versus test failures appears to be due to results only counting if they're of type testcase_test for the failure count. I'll discuss this further with cjohnston.

PASSED: Continuous integration, rev:733
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/294/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/294/rebuild

PASSED: Continuous integration, rev:734
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/295/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/295/rebuild

I guess I just missed the jenkins bot run, but this should be good to go with the changes we previously discussed, and it'll run again.

This should be ready for review with the previously discussed changes. All
tests appear to be passing.

PASSED: Continuous integration, rev:741
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/298/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/298/rebuild

FAILED: Continuous integration, rev:742
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/299/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/299/rebuild

FAILED: Continuous integration, rev:743
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/300/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/300/rebuild

PASSED: Continuous integration, rev:743
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/302/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/302/rebuild

FAILED: Continuous integration, rev:744
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/304/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/304/rebuild

PASSED: Continuous integration, rev:745
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/305/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/305/rebuild

i've been poking around and am generally a +1. We've found one issue where something isn't being calculated correctly. I think I might be the cause of that, so i'll take a lookk

One small bug I just fixed:

 http://paste.ubuntu.com/6927034/

basically if you re-ran a job, we'd count categories from failed runs also. This just counts the latest SmokeResult objects for an image

PASSED: Continuous integration, rev:746
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/307/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/307/rebuild

PASSED: Continuous integration, rev:746
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/308/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dashboard-ci/308/rebuild