Merge lp://qastaging/~nuclearbob/qa-dashboard/categories into lp://qastaging/qa-dashboard
- categories
- Merge into dev
Status: | Merged |
---|---|
Approved by: | Chris Johnston |
Approved revision: | 746 |
Merged at revision: | 719 |
Proposed branch: | lp://qastaging/~nuclearbob/qa-dashboard/categories |
Merge into: | lp://qastaging/qa-dashboard |
Diff against target: |
1021 lines (+727/-23) 13 files modified
common/migrations/0011_add_auth_group.py (+24/-0) common/utils.py (+4/-3) qa_dashboard/settings.py (+7/-0) smokeng/forms.py (+24/-0) smokeng/migrations/0009_auto__add_field_smoketestresult_category.py (+125/-0) smokeng/migrations/__init__.py (+3/-0) smokeng/models.py (+57/-2) smokeng/templates/smokeng/image_overview.html (+18/-0) smokeng/templates/smokeng/test_result_detail.html (+32/-0) smokeng/templates/smokeng/test_results.html (+14/-1) smokeng/tests.py (+380/-14) smokeng/utah_utils.py (+4/-0) smokeng/views.py (+35/-3) |
To merge this branch: | bzr merge lp://qastaging/~nuclearbob/qa-dashboard/categories |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chris Johnston | Approve | ||
PS Jenkins bot | continuous-integration | Approve | |
Max Brustkern (community) | Needs Resubmitting | ||
Joe Talbott | Pending | ||
Review via email: mp+199552@code.qastaging.launchpad.net |
Commit message
Added categorization for automated test failures
Description of the change
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.
PS Jenkins bot (ps-jenkins) wrote : | # |
Chris Johnston (cjohnston) wrote : | # |
review needs-fixing
The first thing I noticed is no tests. :-(
The second thing I noticed, @properties are bad.. :-) This MP took
the 'image_
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.
Andy Doan (doanac) wrote : | # |
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
Max Brustkern (nuclearbob) wrote : | # |
> 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_
> 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.
Chris Johnston (cjohnston) wrote : | # |
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_
> > 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-
Max Brustkern (nuclearbob) 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?
> This MP took the 'image_
> 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?
Joe Talbott (joetalbott) wrote : | # |
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(
Max Brustkern (nuclearbob) wrote : | # |
> 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(
Okay, I figured since SmokeImage.summary was a property, I was working on something similar to that. I can make those back into functions.
Max Brustkern (nuclearbob) wrote : | # |
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.
Chris Johnston (cjohnston) wrote : | # |
Evan would like to add the support so that only members of certain teams can perform these actions.
Max Brustkern (nuclearbob) wrote : | # |
Okay. I'll learn how to work with OpenID and come back once that's ready. Any resources you'd suggest?
Chris Johnston (cjohnston) wrote : | # |
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:/
> 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
Max Brustkern (nuclearbob) wrote : | # |
Cool, thanks.
Max Brustkern (nuclearbob) wrote : | # |
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?
Max Brustkern (nuclearbob) wrote : | # |
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.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:720
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Joe Talbott (joetalbott) wrote : | # |
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-
> Joe Talbott (joetalbott)
>
> For more details, see:
> https:/
>
> 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:/
> You are requested to review the proposed merge of lp:~nuclearbob/qa-dashboard/categories into lp:qa-dashboard.
> === added file 'common/
> --- common/
> +++ common/
> @@ -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.
> +
> +class Migration(
> +
> + def forwards(self, orm):
> + Group.objects.
I think this needs to be orm['django.
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
Max Brustkern (nuclearbob) wrote : | # |
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.
when I try to use orm.
Max Brustkern (nuclearbob) wrote : | # |
Okay, that should be taken care of now.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:721
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Joe Talbott (joetalbott) wrote : | # |
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_
> image_overview,
> @@ -417,6 +419,7 @@
> 'totals': totals,
> 'jenkins_url': jenkins_url,
> 'private_url': private_url,
> + 'categories': result.
> 'table': table,
> 'artifacts': artifacts,
> 'history': _test_history(
> @@ -468,7 +471,6 @@
> 'id',
> ],
> )
> -@require_GET
I think this should be:
@require_
Make sure to import it along with require_GET.
Other than that I think it looks fine.
Joe
Max Brustkern (nuclearbob) wrote : | # |
That should be fixed now.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:722
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Chris Johnston (cjohnston) wrote : | # |
Can we please add, at a minimum testing that the permissions works as well as testing of submitting the form.
Max Brustkern (nuclearbob) wrote : | # |
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?
Chris Johnston (cjohnston) wrote : | # |
Max Brustkern (nuclearbob) wrote : | # |
Awesome, thanks!
Max Brustkern (nuclearbob) wrote : | # |
I've added some additional tests.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:726
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Chris Johnston (cjohnston) wrote : | # |
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://
failures, but there are no failing tests or tests with errors..
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:727
http://
Executed test runs:
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:728
http://
Executed test runs:
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:731
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Max Brustkern (nuclearbob) wrote : | # |
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.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:733
http://
Executed test runs:
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:734
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Max Brustkern (nuclearbob) wrote : | # |
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.
- 710. By Andy Doan
-
[r=Joe Talbott, PS Jenkins bot] fix artifact issues with mega-jobs
A jenkins job may now have artifacts that are specific to a testrun that occurred in a mega-job. As the code currently stands, a crash in one test will wind up being counted for all the test runs. This causes us to show 25 crashes instead of just 1. Additionally, we are showing log files for the job and not just for the run. This adds logic to show the right thing when we are dealing with a mega-job. from Andy Doan
- 711. By Joe Talbot <email address hidden>
-
[r=Andy Doan, PS Jenkins bot] Bootspeed - No longer require unique md5s.
* we aren't using md5s for anything and we've got a constraint on:
- release, variant, arch, build_number from Joe Talbott - 712. By Chris Johnston
-
2014.02.03
- 713. By Andy Doan
-
[r=PS Jenkins bot, Joe Talbott] allow publishing of smoketestresults via REST API
Allow the client to include the UTAH YAML with its SmokeResult object
so that the SmokeTestResult details can also be populated
from Andy Doan
Max Brustkern (nuclearbob) wrote : | # |
This should be ready for review with the previously discussed changes. All
tests appear to be passing.
- 714. By Joe Talbot <email address hidden>
-
[r=Andy Doan] Smoke - Do not include test results for hidden images or results. 1277581 from Joe Talbott
- 715. By Andy Doan
-
[r=Chris Johnston] filter smoketestresult artifacts properly for mega jobs
The logic was already there, but was only used for SmokeResults 1278510 from Andy Doan
- 716. By Chris Johnston
-
2013.02.10
- 728. By Max Brustkern
-
Made migration use orm
- 729. By Max Brustkern
-
Added require_
http_methods - 730. By Max Brustkern
-
Only allowing users who are allowed to triage to submit the form
- 731. By Max Brustkern
-
Added authentication tests
- 732. By Max Brustkern
-
Fixed flake8 warnings and removed extraneous concatenation operators
- 733. By Max Brustkern
-
Added form test
- 734. By Max Brustkern
-
Modified display of uncategorized and form per Chris's suggestions
- 735. By Max Brustkern
-
Removed needless newlines
- 736. By Max Brustkern
-
Changed Failure to Result
- 737. By Max Brustkern
-
Removed print
- 738. By Max Brustkern
-
Updated tests for new flow
- 739. By Max Brustkern
-
Only enabling categories for failures
- 740. By Max Brustkern
-
Categorization should only appear for test failures now
- 741. By Max Brustkern
-
Removed extra context variable
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:741
http://
Executed test runs:
Click here to trigger a rebuild:
http://
- 742. By Max Brustkern
-
Made column able to be blank and null
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:742
http://
Executed test runs:
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:743
http://
Executed test runs:
Click here to trigger a rebuild:
http://
- 743. By Max Brustkern
-
Allowed blank category for old results
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:743
http://
Executed test runs:
Click here to trigger a rebuild:
http://
- 744. By Max Brustkern
-
Added cjohnston's modifications
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:744
http://
Executed test runs:
Click here to trigger a rebuild:
http://
- 745. By Max Brustkern
-
Added forms
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:745
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Andy Doan (doanac) wrote : | # |
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
Andy Doan (doanac) wrote : | # |
One small bug I just fixed:
http://
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
- 746. By Max Brustkern
-
Fixed test rerun issue with patch from doanac
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:746
http://
Executed test runs:
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:746
http://
Executed test runs:
Click here to trigger a rebuild:
http://
Chris Johnston (cjohnston) : | # |
FAILED: Continuous integration, rev:707 /code.launchpad .net/~nuclearbo b/qa-dashboard/ categories/ +merge/ 199552/ +edit-commit- message
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:/
http:// s-jenkins. ubuntu- ci:8080/ job/dashboard- ci/282/
Executed test runs:
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/dashboard- ci/282/ rebuild
http://