Closed Bug 779529 Opened 12 years ago Closed 9 years ago

Implement an easier way for people to file intermittent orange bugs from TBPL

Categories

(Tree Management Graveyard :: TBPL, defect)

Product:
Tree Management Graveyard
Component:
TBPL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: emorley, Unassigned)

References

Details

(Keywords: sheriffing-P1)

Attachments

(1 file, 5 obsolete files)

Next revision
99.42 KB, patch
emorley
: review-
Details | Diff | Splinter Review 
Filing intermittent oranges is both a hassle & requires people to set the correct whiteboard annotations/blocking bug randomorange, or bugs can fall off the radar. I currently use 3-4 different bugzilla template bookmarks for filing oranges, which cut out some of the repetitiveness, but most devs won't have these & they still require a lot of manual work, so I would like to build something into TBPL.

I hope that making it easier to file bugs, will lead to [oranges] being filed sooner (and say even from try runs), which will aid finding root causes (using things like orange seed, bug 669316). In addition, we can make it so that oranges are filed under another user, so the filers aren't put off by knowing they'll be spammed forever.

The rough plan I have in my head:
* TBPL adds a "file orange" link to the annotated summary box
* User is taken to an intermediate page which lets them fill out any parts that are hard to automate (there will be quite a few of these in the initial revision; eg product/component/who to CC)
* User is directed to a custom Bugzilla form (like IT uses) pre-populated with the bug details, where they can do one last check; and then when they submit the bug, it will be filed under TBPLBot's account (and some record kept of whose bugzilla account actually filed, so we can follow up with any questions/to prevent abuse)

I guess we could skip the second step and build all of the above into the custom bugzilla form; but that means writing it all in Perl/will involve the b.m.o team every time we need to make any changes. I'm also not sure how easy it would be to omit the third step and just get the form submitted over bzapi (given that we're using a custom form so presumably would need the API adjusting).

In the last week, jdm has also mentioned he's been working on something similar, but standalone from TBPL (http://www.joshmatthews.net/orange-you-glad/) - which will be useful for ideas. It takes the approach of scraping TBPL's getParsedLog.php for various fields - though if we implement the bug filer into TBPL we'll have direct access to them, which will make things simpler.

Outline of what the bug filer needs to capture/submit (bit of a brain dump, so bear with me):

Product: 
  <When doing manually, I calculate from the locations of bugs in the hg log; guess we could also use test path?>
Component: <as above>

CC: 
  <I CC people who appear in the hg log; though there are often false positives from mass tree refactorings eg MPL2 change>

OS: 
  Easy to match against the standard TBPL fields

Platform: 
  Easy to match against the standard TBPL fields

Whiteboard: 
  [orange] and optionally: {[purple],[red]}

Blocking: 
  bug 'randomorange' and optionally the bug that landed the test/last touched it

Keywords: 
  None, unless 'crash', 'assertion' or 'mlk'

Severity:
  Normal, unless crash/assertion, in which case 'critical'

Summary:

  Intermittent <test filename, or partial path if not clear> | <failure message>

  ...or for crashes:
  Intermittent crash in <test filename, or partial path if not clear> [@ <crash_signature>] ("<any additional assertions or things of note")

  ...or if shutdown/awkward something like :
  Intermittent <OS> talos tp5 crash at <URL> [@ <crash_signature>]
  Intermittent <OS> shutdown crash [@ <crash_signature>]

  ...or for leaks
  Intermittent <test suite name> leak of <size> bytes (<summary of leaking objections shown in annotated summary box, albeit just the top N alphabetically listed>)

Description:
  * Slave
  * Date/time of push/failure
  * Revision hash and tree
  * Brief log URL
  * NEW: Link to TBPL page for that push (currently have to go via the brief log)
  * NEW: Link to MXR entry for test
  * NEW: Who used the bug filer (since filed under TBPLBot)
  * Excerpt of the failure (need to strip out screenshot + reftest dumps, since spammy and often takes the description over the max bugzilla field size). We could always just put the contents of TBPL's annotated summary box (which is less than we normally transpose by hand), but it would be the quickest solution for v1 perhaps.
  
Attachments:
  None, unless log contains screenshots (though not always relevant, so maybe not worth the bugzilla DB bloat), or reftest logs (these make it easier for devs to copy and paste straight into the reftest analyser, rather than having to open the full log).

Crash Signature:
  None, unless crash (duh).
Whiteboard: [sheriff-want]
Meant to add:
We're definitely not going to be able to cover all cases with this feature - I think the main thing is that we just get something that does 90% of the work (a la m-cMerge), rather than trying to aim for perfection. Anything will be better than my bookmarks!

Speaking of which, couple of examples of what I use at the moment...

Firefox for Android [orange]:
https://bugzilla.mozilla.org/enter_bug.cgi?blocked=randomorange&comment=%0D%0A%0D%0A%0D%0A%0D%0A%0D%0A{%0D%0A%0D%0A}&form_name=enter_bug&op_sys=Android&product=Firefox%20for%20Android&rep_platform=ARM&short_desc=Intermittent%20&status_whiteboard=[orange]&target_milestone=---&version=Trunk

Core::JS crasher:
https://bugzilla.mozilla.org/enter_bug.cgi?alias=&assigned_to=general%40js.bugs&blocked=randomorange&bug_file_loc=http%3A%2F%2F&bug_severity=critical&bug_status=NEW&cf_blocking_basecamp=---&cf_blocking_fennec=---&cf_blocking_kilimanjaro=---&cf_crash_signature=[%40%20]&cf_status_esr10=---&cf_status_firefox14=---&cf_status_firefox15=---&cf_status_firefox16=---&cf_status_firefox17=---&cf_tracking_esr10=---&cf_tracking_firefox14=---&cf_tracking_firefox15=---&cf_tracking_firefox16=---&cf_tracking_firefox17=---&comment=%0D%0A%0D%0A%0D%0A%0D%0A%0D%0A{%0D%0A%0D%0A}&component=JavaScript%20Engine&contenttypeentry=&contenttypemethod=autodetect&contenttypeselection=text%2Fplain&data=&defined_groups=1&dependson=&description=&flag_type-203=X&flag_type-325=X&flag_type-37=X&flag_type-4=X&flag_type-41=X&flag_type-5=X&flag_type-607=X&flag_type-720=X&flag_type-721=X&flag_type-737=X&flag_type-775=X&flag_type-780=X&flag_type-781=X&form_name=enter_bug&keywords=crash%2C%20&maketemplate=Remember%20values%20as%20bookmarkable%20template&op_sys=All&priority=--&product=Core&qa_contact=&rep_platform=All&requestee_type-203=&requestee_type-325=&requestee_type-4=&requestee_type-41=&requestee_type-5=&requestee_type-563=&requestee_type-607=&requestee_type-781=&short_desc=Intermittent%20crash%20in%20[%40%20]&status_whiteboard=[orange]&target_milestone=---&version=Trunk
A word of note: using the bzapi is really easy; that's what Orange You Glad does (I tested it on landfill and it worked), so you could just crib the code from there.
Of course, the downside of using the bzapi is that when the server hosting it goes down (which is not associated with bugzilla), the tool is unusable. However, iirc tbpl also gets in the same state when that happens, so there wouldn't be much of a change there.
(In reply to Josh Matthews [:jdm] from comment #2)
> A word of note: using the bzapi is really easy; that's what Orange You Glad
> does (I tested it on landfill and it worked), so you could just crib the
> code from there.

With a custom form?!
ie for this part:

> and then when they
> submit the bug, it will be filed under TBPLBot's account (and some record
> kept of whose bugzilla account actually filed, so we can follow up with any
> questions/to prevent abuse)
Filing under TBPLbot's account is easy. Adding more information to the summary is just string concatenation. I find your surprise surprising, unless I'm misunderstanding what you mean by a custom form.
Ah sorry it would seem I missed out a key piece of what I was thinking.

When discussing this with philor (/glob?) the potential for spam bug creation was raised, so doing it server-side with no authentication (a la the current starring process) was deemed at risk of abuse.

As such, we would either have to add LDAP support to TBPL's bug filer (given that the sheriffpass wouldn't be viable, since we want all devs to be able to use this tool), or else use Bugzilla for authentication. A custom form (like the custom form used for IT requests) would allow us to ensure people were logged into a b.m.o account (and record the username), whilst still filing under the TBPLbot user.

The only other option would be to require people's b.m.o credentials be entered into TBPL and verify them server-side before doing the bug filing. However I had ruled this out initially given we'd presumably need a full sec-review of tbpl before proceeding / people may be dubious about submitting s-s account details outside of bugzilla.

Open to ideas; I've probably just missed an easy solution! :-)
(In reply to Ed Morley [:edmorley] from comment #7)
> As such, we would either have to add LDAP support to TBPL's bug filer (given
> that the sheriffpass wouldn't be viable, since we want all devs to be able
> to use this tool), or else use Bugzilla for authentication. A custom form
> (like the custom form used for IT requests) would allow us to ensure people
> were logged into a b.m.o account (and record the username), whilst still
> filing under the TBPLbot user.

from a bmo implementation point of view, we'd use the bug_end_of_create hook to alter the bug's reporter.  this means the bug will initially be created as the authenticated bugzilla user, but we'd ninja the reporter before the transaction is committed.
(Another brain dump)

Few different scenarios:

1) Single new failure
2) Multiple failures, only one new
3) Multiple failures, several new - but deemed related and so filing under one umbrella bug
4) Multiple failures, several new - but deemed unrelated and so filing separately

#1 & #2 are the most common - and the solution to #2 (eg checkboxes to exclude some of the failures) will fix #4 since you'd just run the filer twice.
(In reply to comment #7)
> Ah sorry it would seem I missed out a key piece of what I was thinking.
> 
> When discussing this with philor (/glob?) the potential for spam bug creation
> was raised, so doing it server-side with no authentication (a la the current
> starring process) was deemed at risk of abuse.

People worried about the same thing when I first created tbplbot.  To this day nobody has attempted to abuse it.  Anyone who wishes to abuse bugzilla can just use bzapi directly.  This doesn't make things any easier than they already are.
Sure, you can create multiple throwaway email addresses, create multiple bugzilla accounts from them (because they will be disabled, and you'll need more than one), and use them to create spam bugs directly through bzapi.

Or, you can use this to create them with an account that we won't want to disable.

tbplbot doesn't currently offer any really interesting way to abuse it, since the most you could do is one time per failure spam every bug that matches the failure, though it would get more interesting if you could spam thousands of unrelated bugs by just posting bug numbers, through bug 764887.

This is actually less abuse-prone, presuming that all it will be able to do is create one bug per TEST-UNEXPECTED-*, making it more akin to tbplbot as he is than the asking-for-it version that would comment on any (or every) bug.

When I shout ABUSE! ABUSE! I don't mean there is no way that this can be done, I mean that we need to think from the start about how to limit the potential and the interestingness of abusing it, by doing things here like only creating one bug per unexpected in a given log and then not offering to create any more the same way tbplbot won't offer to restar a starred build, and doing things there like only commenting on [orange] bugs.
Note to self:
Remember to surface an easy way to ascertain if the test has been recently added to the tree (and as such should possibly be backed out instead) - either fetch the date of first hglog entry, or else just show the hg log. Perhaps the latter given v1 may not support suggesting who to CC automatically and so will need to show the hg log anyway.
Keywords: sheriffing-P1
Whiteboard: [sheriff-want]
I think I'll try to work on this soonish.
Assignee: nobody → kwierso
:-D
Attached patch wip (obsolete) — — Splinter Review 
So, this is the start of what I'm doing. It adds a "file bug" button below the retrigger/cancel buttons for a job that is either red or orange on tbpl. 

When you click the button, it prompts you for whether you want to file a bug for that line in the summary. (This is horrible, I know... just wanted to get something that worked.) If you agree to file that line, it opens up Bugzilla's new bug form with the bug's summary line pre-filled with the line from the tbpl summary.

Now that I've seen this bug and read the suggestion for popping up a dialog to get more information before passing it on to Bugzilla, I'll probably work on that next.

Just uploading this for initial feedback from Ed on the placement of the new button...
Attachment #777474 - Flags: feedback?(emorley)
>+ var url = encodeURI("https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox&short_desc=" + i);

you should also set format=__standard__ to ensure people use the standard form (instead of guided or a custom per-product form).
Sorry for the delay, will get to this tomorrow :-)
Comment on attachment 777474 [details] [diff] [review]
wip

Review of attachment 777474 [details] [diff] [review]:
-----------------------------------------------------------------

Really sorry for the delay!

::: js/UserInterface.js
@@ +1401,5 @@
>  
>      BuildAPI.cancelRevision(this.treeInfo.buildbotBranch, rev, onSuccess, onFailure, onTimeout);
>    },
> +  
> +  _fileBugButtonClick: function UserInterface__fileBugButtonClick(fileBugButton, requestOrBuildID) {

The first parameter isn't used - can remove.

@@ +1405,5 @@
> +  _fileBugButtonClick: function UserInterface__fileBugButtonClick(fileBugButton, requestOrBuildID) {
> +    // For each (non-bug-suggestion) line in the job's summary, potentially open the enter_bug page for the line
> +    var summary = document.getElementById("details").getElementsByClassName("summary")[0].innerHTML.split("<br>\n");
> +    for(var i of summary) {
> +      // Strip out the bug suggestion lines, since this isn't an existing bug

What about failures for which there was a suggestion, but it wasn't the correct one?

@@ +1407,5 @@
> +    var summary = document.getElementById("details").getElementsByClassName("summary")[0].innerHTML.split("<br>\n");
> +    for(var i of summary) {
> +      // Strip out the bug suggestion lines, since this isn't an existing bug
> +      if(i.split("<span")[0] != "" && i.split('" data-status')[0] != "") {
> +        // Only offer to file for lines that mention TEST-UNEXPECTED. Is that okay?

I think we need to just cycle through them all, given PROCESS-CRASH and then the long tail of harness errors.

@@ +1409,5 @@
> +      // Strip out the bug suggestion lines, since this isn't an existing bug
> +      if(i.split("<span")[0] != "" && i.split('" data-status')[0] != "") {
> +        // Only offer to file for lines that mention TEST-UNEXPECTED. Is that okay?
> +        if(i.search("TEST-UNEXPECTED") >= 0) {
> +          // This probably sucks if there's a lot of lines of failures. Open to other ideas.

Perhaps we just need to add the buttons to the annotated summary line? That way you just pick the relevant line and select "File a new bug" ?

@@ +1412,5 @@
> +        if(i.search("TEST-UNEXPECTED") >= 0) {
> +          // This probably sucks if there's a lot of lines of failures. Open to other ideas.
> +          if(window.confirm("File a bug for <<< " + i + " >>> ?")) {
> +            // Bug 894071 means I can't just use the summary field, I have to provide a product. 
> +            // Picking Firefox for now because it Just Works[tm]

I'd suggest something like:
* "File new bug" link on the annotated summary line.
* This takes you to a TBPL page (or even just a popup panel) with various orange fields filled out from the log info + ability to select product manually (in v1 we could link to the MXR page for the test, which people can use to lookup product/component; in the future we could handle this more intelligently eg using moz.build info)
* This page then files the new bug using bzapi.
Attachment #777474 - Flags: feedback?(emorley)
Flags: needinfo?(emorley)
Quick recap of chat with Wes:

In order to avoid cluttering the current TBPL UI & also to ensure we have pieces that can be reused with treeherder, we're going to try the following:
* Add the ability to retrieve a single job's metadata+log excerpt from the TBPL DB using the job ID.
* In a subdirectory within the TBPL repo, add a standalone mainly-frontend app that calls this new backend piece.
* The standalone app then processes the metadata to pre-populate most of the bug fields, a human specifies product etc, then they are redirected to bugzilla, with the various fields populated.

Note: For now we've decided to avoid filing the bug using bzapi and the tbplbot credentials, since now that we have the "ignore all bugmail" checkbox, spam is less of an issues (and it means we can avoid the problem of potential abuse).

I'll file a bug for the backend piece, the rough object it will return is:
{
    "_id": "26222910",
    "buildername": "Rev5 MacOSX Mountain Lion 10.8 mozilla-central debug test mochitest-5",
    "slave": "talos-mtnlion-r5-051",
    "revision": "1e381c91885d",
    "result": "testfailed",
    "branch": "mozilla-central",
    "log": "http:\/\/ftp.mozilla.org\/pub\/mozilla.org\/firefox\/tinderbox-builds\/mozilla-central-macosx64-debug\/1375811013\/mozilla-central_mountainlion-debug_test-mochitest-5-bm80-tests1-macosx-build181.txt.gz",
    "starttime": "1375806849",
    "endtime": "1375808391",
    "excerpt": {
        "1" : "TEST-UNEXPECTED-FAIL | Shutdown | Exited with code -20 during test run",
        "2" : "TEST-UNEXPECTED-FAIL | Foo | bar"
    },
    "briefLog": "16:51:16     INFO -  177454 INFO Slowest: 40768ms - /tests/layout/style/test/test_transitions_per_property.html\n16:51:16     INFO -  177455 INFO SimpleTest FINISHED\n16:51:16     INFO -  177456 INFO TEST-INFO | Ran 1 Loops\n16:51:16     INFO -  177457 INFO SimpleTest FINISHED\n16:51:16     INFO -  NOTE: child process received `Goodbye', closing down"
}

(It will just need the job ID specified as parameter 'id').
Depends on: 902272
Note that the 'Whiteboard:' and 'Blocking:' attributes mentioned in comment 0 are no longer used, since they've been replaced by the intermittent-failure keyword :-)
So this version is kinda almost working, and I'm uploading this as a checkpoint of where I'm at.

Since bug 902272 is not yet resolved, I'm just using the sample JSON that Ed provided in comment 19 to pull data from. Once we get that bug resolved, it should be easy enough to switch to requesting from it instead of this static JSON object.

This version doesn't actually add anything to TBPL itself, it's a standalone page (that should have an "?id=[jobID]" in the URL once we can actually pull the live data). I guess the next step is to merge it with the previous wip that adds a button to the job details section of TBPL so that that button opens this new page with the job id pre-appended. 

Given the metadata from comment 19, this page will parse out the job's details like builder/slave/etc, then present a list of the log summary's failure lines, each next to a checkbox. If you check a checkbox next to a line, it will mark it for inclusion in the bug you're about to file.

By default, the page tries to figure out what platform and hardware the job was running on. There's a checkbox to switch those to "All/All" if platform isn't relevant or they apply to all platforms.

Under that is a dropdown list of Products that were fetched from Bugzilla via bzAPI, with some of the more relevant products filtered toward the top of the list. 

You have to select at least one line from the log summary and a specific product before the submit button will be enabled.

When you click the Submit button, the page generates the URL that contains all of the information you have selected so that when you visit that URL, the bug submission form is pre-filled as much as possible.

(Right now, it only prints out the link, and leaves it to the user to click.)
Attachment #777474 - Attachment is obsolete: true
Attachment #787150 - Flags: feedback?(emorley)
Bug 902681 might help make things more enticing for getting non-sheriffs to file these bugs, though I don't think it'd BLOCK this bug...
Attached patch tbplfiler.diff (obsolete) — — Splinter Review 
> I guess the next step is to merge it with the previous wip
> that adds a button to the job details section of TBPL so that that button
> opens this new page with the job id pre-appended. 
This version adds this to UserInterface.js so when you click the little bugzilla icon, it opens the new bug filer tool in a new tab.
Attachment #787150 - Attachment is obsolete: true
Attachment #787150 - Flags: feedback?(emorley)
Attachment #787166 - Flags: feedback?(emorley)
Attached patch Current state of the patch (obsolete) — — Splinter Review 
Okay, this should be good for at least a first review.

With this patch, an icon is added to the job detail section for each non-green job on TBPL. When this icon is clicked, it opens up a new tab/window to this new bug filing tool.

The filer first queries the bzAPI to get a list of products on Bugzilla, and sorts some of the more relevant products up to the top of the list.

Using the job ID, it queries TBPL's new getJob.php to get the job's metadata.

The filer fills out some fields on the page about the Builder, Slave, Revision/Branch, Result, and links to the brief and full logs.

Each line from the log excerpt gets a checkbox and is put in a list.

The page's script tries to figure out what platform and hardware this job was running on from the Builder string.



At this point, the user has to do things to proceed. You need to select at least
one line from the log excerpt and you need to choose a product from the list before the submit button will be enabled. 

You can optionally uncheck the checkbox for including the platform and hardware information in the submission (if you do this, it will choose "All/All" instead).

You can optionally choose to receive bugmail for this bug you're creating. (This will set the hidden "never email me about this bug" field if you don't check it.) ((I'm indifferent on what the default should be for this checkbox. Making bugmail opt-in might encourage non-sheriffs to use the tool and file intermittent bugs; making bugmail opt-out would probably be what sheriffs would usually want to help make sure the bugs are followed up on by the right people.))


When you click the submit button, it redirects the page to bugzilla using the URL the tool generated that includes all of the information collected from the page.
Attachment #787166 - Attachment is obsolete: true
Attachment #787166 - Flags: feedback?(emorley)
Attachment #787656 - Flags: review?(emorley)
Attached patch Current state of the patch (obsolete) — — Splinter Review 
Oops, forgot to qref...

I'm unsure what I could take out of the newly added images/css files, I just blatantly stole them from mcMerge.
Attachment #787656 - Attachment is obsolete: true
Attachment #787656 - Flags: review?(emorley)
Attachment #787666 - Flags: review?(emorley)
I'm wondering if it'd be better to have textboxes for the new bug's summary and description fields on the new tool's page, so the user can edit them before moving over to bugzilla. Some lines from the excerpt could be really long and might not fit in the inputs on bugzilla, so it'd be good to let the user cut them down at a good point instead of just being cut off at the maxlength for the field. (The tool also just uses the first selected excerpt line as the bug summary, and I guess there might be cases where you'd want a later line as the summary instead?)
/Really/ sorry for the delay, looking at this now.
Comment on attachment 787666 [details] [diff] [review]
Current state of the patch

On the whole, this is great, thank you Wes :-)

I think the best way forwards after speaking to dburns is for me to make a few quick comments for the obvious things & then for us to just get this landed and we can iterate from there, so this doesn't hold you up any more (really sorry).

I used https://tbpl-dev.allizom.org/php/getParsedLog.php?id=28392092&tree=mozilla-inbound as an example.

The resultant bug form had values:

Summary:
Intermittent TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | application timed out after 330 seconds with no output

Description:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | application timed out after 330 seconds with no output


Job: mochitest-other
Slave name: talos-mtnlion-r5-037
Revision: 9c195fd74f04
Brief log: https://tbpl-dev.allizom.org/php/getParsedLog.php?id=28392092&tree=mozilla-inbound
TBPL Link: https://tbpl-dev.allizom.org/?tree=mozilla-inbound&rev=9c195fd74f04
}

Please can we make the summary for this case:
Intermittent content/events/test/test_bug617528.xul | application timed out after 330 seconds with no output

And to the description I think we need to:
* Add datetime of test run
* Whether opt/debug etc
* The slave type (eg "Rev5 MacOSX Mountain Lion 10.8").
* Which tree "Tree: mozilla-inbound"
* s/Job/Job type/
* The excerpt of the failure (the TBPL highlight lines and some context before... let's just start with say 10 lines prior to the selected highlight lines, yeah? We'll need to strip out the screenshot |data:| lines. Then for v2 we can maybe add the ability in the filer UI to choose what context by highlighting the log etc).
* s/Brief log/Log/
* Add the MXR link mentioned in comment 0 (split the testname from the filepath, and if no slash just use the whole string but don't use MXR file search, use the normal search, eg for "leakcheck" etc). 

So maybe something like this?
{
Job type: mochitest-other opt
Platform: Rev5 MacOSX Mountain Lion 10.8
Slave name: talos-mtnlion-r5-037
Job started at: 2013-09-25 23:49:48 PDT
Tree: mozilla-inbound
Revision: 9c195fd74f04

Log: https://tbpl-dev.allizom.org/php/getParsedLog.php?id=28392092&tree=mozilla-inbound
TBPL link: https://tbpl-dev.allizom.org/?tree=mozilla-inbound&rev=9c195fd74f04
MXR search: http://mxr.mozilla.org/mozilla-central/find?string=test_bug617528.xul&tree=mozilla-central

Failure excerpt:
{
23:53:14     INFO -  1376 INFO TEST-PASS | chrome://mochitests/content/chrome/content/events/test/test_bug602962.xul | Height should be resized
23:53:14     INFO -  1377 INFO TEST-END | chrome://mochitests/content/chrome/content/events/test/test_bug602962.xul | finished in 320ms
23:53:14     INFO -  1378 INFO TEST-START | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul
23:53:14     INFO -  OpenGL version detected: 210
23:53:14     INFO -  1379 INFO TEST-PASS | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | expected event.defaultPrevented to be true (1)
23:53:14     INFO -  1380 INFO TEST-PASS | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | expected event.target.localName to be 'input' (1)
23:53:14     INFO -  1381 INFO TEST-PASS | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | expected event.originalTarget.localName to be 'div' (1)
23:53:14     INFO -  1382 INFO TEST-PASS | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | expected event.defaultPrevented to be false (2)
23:53:14     INFO -  1383 INFO TEST-PASS | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | expected event.target.localName to be 'input' (2)
23:53:14     INFO -  1384 INFO TEST-PASS | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | expected event.originalTarget.localName to be 'div' (2)
23:58:44  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | application timed out after 330 seconds with no output
}
}

For the filing page itself, I'd like it if we could add the same MXR link as above - or even better, try and generate the hg log URL by stripping off the known mochitest/xpcshell/... URI prefixes and appending "https://hg.mozilla.org/<tree>/log/<rev>/" etc.
Attachment #787666 - Flags: review?(emorley)
(In reply to Wes Kocher (:KWierso) from comment #26)
> I'm wondering if it'd be better to have textboxes for the new bug's summary
> and description fields on the new tool's page, so the user can edit them
> before moving over to bugzilla. Some lines from the excerpt could be really
> long and might not fit in the inputs on bugzilla, so it'd be good to let the
> user cut them down at a good point instead of just being cut off at the
> maxlength for the field. (The tool also just uses the first selected excerpt
> line as the bug summary, and I guess there might be cases where you'd want a
> later line as the summary instead?)

Yeah this might be a good idea - shall we see how it goes once we strip off the superfluous text (comment 28)? We ideally also need to add more than just the first line to the summary, eg for:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | foo foo foo
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/events/test/test_bug617528.xul | bar bar bar

We'd set the summary to:
content/events/test/test_bug617528.xul | foo foo foo | bar bar bar
(In reply to Ed Morley [:edmorley UTC+1] from comment #29)
> We'd set the summary to:
> content/events/test/test_bug617528.xul | foo foo foo | bar bar bar

Sorry I meant:
Intermittent content/events/test/test_bug617528.xul | foo foo foo | bar bar bar
(In reply to Ed Morley [:edmorley UTC+1] from comment #28)
> For the filing page itself, I'd like it if we could add the same MXR link as
> above - or even better, try and generate the hg log URL by stripping off the
> known mochitest/xpcshell/... URI prefixes and appending
> "https://hg.mozilla.org/<tree>/log/<rev>/" etc.

This would also help the user pick a component.
Comment on attachment 787666 [details] [diff] [review]
Current state of the patch

Review of attachment 787666 [details] [diff] [review]:
-----------------------------------------------------------------

::: filer/css/style.css
@@ +150,5 @@
> +  margin-bottom: 0px;
> +}
> +
> +.backoutHeader {
> +  color: red; 

nit: trailing whitespace

@@ +231,5 @@
> +  left: 0;
> +  bottom: 0;
> +  right: 0;
> +  opacity: .80;
> +  filter: Alpha(Opacity=80); 

nit: trailing whitespace

::: filer/filer.js
@@ +1,1 @@
> +let metadata;

Can we "use strict"; please? :-)

@@ +1,3 @@
> +let metadata;
> +let brieflog;
> +let fulllog;

Since this is in the global scope, var is more correct over let, unless I'm mis-remembering?

@@ +1,5 @@
> +let metadata;
> +let brieflog;
> +let fulllog;
> +
> +let tbplRoot = "https://tbpl.mozilla.org/";

s/tbplRoot/prodTBPLRoot/ maybe? (to make it clear we're intentionally referencing production TBPL vs the local instance, similar to the convention in TBPL's config?)

@@ +2,5 @@
> +let brieflog;
> +let fulllog;
> +
> +let tbplRoot = "https://tbpl.mozilla.org/";
> +tbplRoot = "https://tbpl-dev.allizom.org/"; // Delete this line when 902272 is pushed live

This can now be removed :-)

@@ +5,5 @@
> +let tbplRoot = "https://tbpl.mozilla.org/";
> +tbplRoot = "https://tbpl-dev.allizom.org/"; // Delete this line when 902272 is pushed live
> +
> +/*
> + *  Fetch BMO's list of products

Can we wrap this in a function and call separately? :-)

@@ +7,5 @@
> +
> +/*
> + *  Fetch BMO's list of products
> + */
> +let bmoConfig;

This appears unused?

@@ +8,5 @@
> +/*
> + *  Fetch BMO's list of products
> + */
> +let bmoConfig;
> +

Can we wrap this in a function and call separately? :-)

@@ +19,5 @@
> +      let jsonConfig = JSON.parse(bzxhr.responseText);
> +      let productArray = [];
> +      let productSelect = document.getElementById("productSelect");
> +
> +      for(i in jsonConfig.products) {

'i' isn't declared + missing space between for and bracket

@@ +25,5 @@
> +      }
> +      productArray.push("-----");
> +      productArray.sort(sortProductArray);
> +
> +      for(i in productArray) {

'i' isn't declared + missing space between for and bracket

@@ +34,5 @@
> +      productSelect.removeAttribute("disabled");
> +      productSelect.addEventListener("change", updateInputStatus);
> +    }
> +  }
> +}

Missing closing semi-colon.

@@ +52,5 @@
> +  document.getElementById("submit").addEventListener("click", submitBug);
> +}
> +
> +/*
> + *  Fetch this job's metadata (until 902272 is finished, this just uses the sample job)

Think the comment needs updating :-)

@@ +64,5 @@
> +    if (xhr.readyState === 4) {
> +      if (xhr.status === 200) {
> +        let logLineSelectionDiv = document.getElementById("loglineselection");
> +        // Fill in the job's details into the page
> +        metadata = JSON.parse(xhr.responseText); 

nit: trailing whitespace

@@ +115,5 @@
> +    return -1;
> +  } else {
> +    if(a == "-----") {
> +      return 1;
> +    } else { 

This else is redundant.

nit: trailing whitespace

@@ +119,5 @@
> +    } else { 
> +      return 2;
> +    }
> +  }
> +}

Missing closing semi-colon.

@@ +135,5 @@
> +      anyChecked = true;
> +    }
> +  }
> +  if(anyChecked) {
> +    anyChecked = anyChecked && productSelect.selectedIndex > 0 && 

nit: trailing whitespace

@@ +140,5 @@
> +                 productSelect[productSelect.selectedIndex].textContent != "-----";
> +  }
> +
> +  document.getElementById("submit").disabled = !anyChecked;
> +}

Missing closing semi-colon.

@@ +200,5 @@
> +    encodeURIComponent("\nBrief log: " + brieflog);
> +  submissionstring = submissionstring + 
> +    encodeURIComponent("\nTBPL Link: " + tbplRoot + "?tree=" + 
> +    metadata.branch + "&rev=" + metadata.revision);
> +

nit: Several instances of trailing whitespace above :-)

@@ +214,5 @@
> +  } else if(/Linux/.test(builderstring) || /Ubuntu VM/.test(builderstring)) {
> +    osstring = "Linux";
> +  } else if(/Windows XP/.test(builderstring)) {
> +    osstring = "Windows XP";
> +  } else if(/Windows 7/.test(builderstring) || /WINNT 5.2/.test(builderstring)) {

The period in WINNT 5.2 needs to be escaped :-)

@@ +238,5 @@
> +  let platformString = "&rep_platform=" + platform + "&op_sys=" + osstring;
> +
> +  // Add the intermittent keyword by default (should this be optional?)
> +  // If one of the included summary lines mentions a crash, add the crash keyword and up the severity
> +  let keywords = "&keywords=intermittent-failure"

Missing closing semi-colon.

@@ +248,5 @@
> +  if(!document.getElementById("bugmaildesired").checked) {
> +    bugmail = "&bug_ignored=1";
> +  }
> +  
> +  window.location = "https://bugzilla.mozilla.org/enter_bug.cgi?format=__standard__&product=" + 

nit: trailing whitespace x2

@@ +251,5 @@
> +  
> +  window.location = "https://bugzilla.mozilla.org/enter_bug.cgi?format=__standard__&product=" + 
> +    productString + "&comment=" + submissionstring + "&short_desc=" + summarystring +
> +    platformString + keywords + bugmail;
> +}

Missing closing semi-colon.

::: filer/index.html
@@ +1,1 @@
> +<html>

Please can you add an initial <!DOCTYPE html>

@@ +1,2 @@
> +<html>
> +  <head>

Please can we add a <title> and <meta charset="utf-8"> ? :-)
nit: Bonus points if we set a favicon (let's just steal bugzilla's for now, similar to Harth's fileit).

@@ +1,4 @@
> +<html>
> +  <head>
> +    <script type="application/javascript;version=1.7" src="filer.js"></script>
> +    

nit: trailing whitespace plus the ones below :-)

@@ +44,5 @@
> +    <div id="loglineselection"></div>
> +    
> +    <h2> Select a product and Submit: </h2>
> +    <div>
> +      <input id="isPlatformImportant" type="checkbox" checked="true"/>

s/checked="true"/checked/

@@ +54,5 @@
> +    </div>
> +    <select id="productSelect" disabled="true">
> +      <option>SELECT A PRODUCT</option>
> +    </select>
> +    <input type="button" id="submit" disabled="true" value="Submit"/>

s/disabled="true"/disabled/

::: js/UserInterface.js
@@ +1400,5 @@
>      cancelAllButton.style.display = 'none';
>  
>      BuildAPI.cancelRevision(this.treeInfo.buildbotBranch, rev, onSuccess, onFailure, onTimeout);
>    },
> +  

nit: trailing whitespace
Comment on attachment 787666 [details] [diff] [review]
Current state of the patch

Review of attachment 787666 [details] [diff] [review]:
-----------------------------------------------------------------

::: filer/filer.js
@@ +65,5 @@
> +      if (xhr.status === 200) {
> +        let logLineSelectionDiv = document.getElementById("loglineselection");
> +        // Fill in the job's details into the page
> +        metadata = JSON.parse(xhr.responseText); 
> +        document.getElementById("jobID").firstElementChild.textContent = metadata._id;

I think we can ditch the job # here (that ends up in the <span></span> after "Job Details:") - it's not really interpretable by humans.

@@ +69,5 @@
> +        document.getElementById("jobID").firstElementChild.textContent = metadata._id;
> +        document.getElementById("buildername").firstElementChild.textContent = metadata.buildername;
> +        document.getElementById("slave").firstElementChild.textContent = metadata.slave;
> +        document.getElementById("revisionandbranch").firstElementChild.textContent = metadata.branch + " " + metadata.revision;
> +        document.getElementById("result").firstElementChild.textContent = metadata.result;

Please can we add the additional metadata that will end up in the description (comment 28).

@@ +111,5 @@
> + */
> +var sortProductArray = function(a,b) {
> +  if(a == "Firefox" || a == "Firefox for Metro" || a == "Core" || a == "Boot2Gecko" ||
> +     a == "Toolkit" || a == "Testing" || a == "Firefox for Android" || a == "mozilla.org") {
> +    return -1;

Could we add the Addon-SDK product and new releng products to this list, and remove mozill.org?
Also, please can we sort alphabetically within each of the "top products" and "other products" :-)

@@ +209,5 @@
> +  let platform = "All";
> +
> +  if(/OS X/.test(builderstring) || /OSX/.test(builderstring)) {
> +    osstring = "Mac OS X";
> +    platform = "x86_64";

Let's move this down to the rest of the platform logic.

@@ +228,5 @@
> +  } else if(/X86/.test(builderstring)) {
> +    platform = "x86";
> +  } else if(/Armv6/.test(builderstring)) {
> +    platform = "ARM";
> +  }

We need to also cater for Windows here (currently sets "All" instead of "x86" for WinXP 32bit etc).

@@ +233,5 @@
> +
> +  // ... but only include them if they're relevant. "All/All", otherwise.
> +  let platformImportant = document.getElementById("isPlatformImportant").checked;
> +  osstring = platformImportant ? osstring : "All";
> +  platform = platformImportant ? platform : "All";

This can be replaced by placing the regex matches above inside a |if (platformImportant)|, since they are instantiated to "All" anyway.

@@ +236,5 @@
> +  osstring = platformImportant ? osstring : "All";
> +  platform = platformImportant ? platform : "All";
> +  let platformString = "&rep_platform=" + platform + "&op_sys=" + osstring;
> +
> +  // Add the intermittent keyword by default (should this be optional?)

No, this should always be present (and people can always edit the completed bug form anyway :-)

::: filer/index.html
@@ +28,5 @@
> +  </head>
> +
> +  <body onload=start()>
> +  <div class="ctr">
> +      <h1 id="title">Intermittent Filer</h1>

nit: "Intermittent Bug Filer" ? :-)

@@ +30,5 @@
> +  <body onload=start()>
> +  <div class="ctr">
> +      <h1 id="title">Intermittent Filer</h1>
> +  </div>
> +    <h2 id="jobID"> Job Details: <span></span></h2>

I think we can ditch the job # here (in the <span></span>) - it's not really interpretable by humans.

@@ +39,5 @@
> +      <div id="result">Result: <span></span></div>
> +      <span id="brieflog"><a>Brief log</a></span> || <span id="fulllog"><a>Full log</a></span>
> +    </div>
> +    
> +    <h2> Select log lines to file: </h2>

nit: s/file/include/ maybe?

@@ +42,5 @@
> +    
> +    <h2> Select log lines to file: </h2>
> +    <div id="loglineselection"></div>
> +    
> +    <h2> Select a product and Submit: </h2>

Maybe make this heading "Options:" ?

@@ +54,5 @@
> +    </div>
> +    <select id="productSelect" disabled="true">
> +      <option>SELECT A PRODUCT</option>
> +    </select>
> +    <input type="button" id="submit" disabled="true" value="Submit"/>

Please can you place the submit button on the next line (currently on the same line as the select).

::: js/UserInterface.js
@@ +1445,5 @@
>          (UserInterface._allowCancel(result) ?
>            '<img tabindex="0" src="images/tango-process-stop.png" title="Cancel Job"' +
>            ' onclick="UserInterface._cancelButtonClick(this, \'' + requestOrBuildID + '\')">'
> +        : '') +
> +        (result.state == "testfailed" || result.state == "busted" ?

We need to use:
(result.state != 'success')
...since otherwise we miss 'exception' etc.

We could technically also exclude 'usercancel', but I think there may be the odd time a run is cancelled and there also happens to need to be a bug filed from the failures that occurred prior to it ending.

@@ +1447,5 @@
>            ' onclick="UserInterface._cancelButtonClick(this, \'' + requestOrBuildID + '\')">'
> +        : '') +
> +        (result.state == "testfailed" || result.state == "busted" ?
> +          '<img tabindex="0" src="images/bz.png" title="File a Bug"' +
> +          ' onclick="UserInterface._fileBugButtonClick(this, \'' + requestOrBuildID + '\')">' : '')

This is currently inside the Config.selfServeURL ternary - however the bug filer doesn't interact with self-serve. Please can you move this after the end of that conditional :-)
Attached patch Next revision — — Splinter Review 
So this addresses most of your review concerns, though it still isn't ready to land and get deployed.

I don't have it including the start time because I'm having weird things happen when I try to convert the timestamp from what the job information gives me to something human readable.

It also doesn't include the link to MXR for the files because parsing that out was more than I wanted to deal with tonight.

The summary filters out the "TEST-UNEXPECTED-FAIL" part as well as "chrome://mochitest", and leaves room for adding more things to filter out later.

The description now mentions opt/debug on the platform line, and includes the full log, not the brief log.

It also tries to include some lines around the failures as context in the description, with a variable to control the number of lines included up at the top of filer.js. It doesn't include lines that include data:image URIs, since those would be way too long to submit to Bugzilla. It pops up an alert when you click "submit" that the screenshot was stripped and that you'd need to go attach it manually. Maybe in the future it can open the data: URIs up in new tabs for us to speed things up?

I'm actually unsure if context should be automatically added, because eve without the screenshot data, it pushes the URL string for the initial request to bugzilla really close to the character limit, which then just outright fails to load bugzilla.


This also doesn't coalesce the failure lines if they're in the same file.

Just wanted to upload this as a checkpoint for addressing your review, Ed.

Hopefully going forward, we won't be replying to each other with two months between interactions...
Attachment #787666 - Attachment is obsolete: true
Attachment #832031 - Flags: review?(emorley)
Comment on attachment 832031 [details] [diff] [review]
Next revision

Review of attachment 832031 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks great, thank you :-)

Not r+ quite yet due to ending up with markup in the bug description from the getParsedLog.php content - once that is fixed happy to land and iterate from there :-) \o/

How often have you hit the URL length limit with the 5 lines of context? Maybe we could trim each lines if it is greater than N characters?

Also, something that might be nice to do at some point (followup is fine), is make the version field default to 'trunk' unless a release branch (in which case leave it unselected, to force the user to choose it), or else Firefox OS/Release Engineering etc etc (in which case use 'unspecified' if the milestones don't match anything sensible).

::: filer/filer.js
@@ +86,5 @@
> +        xhr2.onerror = function (e) {
> +          console.error(xhr2.statusText);
> +        };
> +        xhr2.send(null);
> +      

nit: Trailing whitespace

@@ +96,5 @@
> +        // Add links to the job's brief and full logs
> +        fulllog = prodTBPLRoot + "php/getParsedLog.php?full=1&id=" + metadata._id + "&tree=" + metadata.branch;
> +        document.getElementById("fulllog").firstElementChild.href = fulllog;
> +
> +        // For each of the lines of TBPL summaries, add a checkbox to next to it to mark for inclusion in the bug

When testing, I ended up with this on the bugzilla file a bug page:
{
Failure excerpt: 
</strong><strong id="error1628">06:16:53     INFO -  14604 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/dom/ranges/test_Range-insertNode.html | 36,21: resulting DOM for range [docfrag, 0, docfrag, 0], node foreignDoctype
</strong><strong id="error1629">06:16:53     INFO -  14605 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/dom/ranges/test_Range-insertNode.html | 36,21: resulting range position for range [docfrag, 0, docfrag, 0], node foreignDoctype
</strong><strong id="error1630">06:16:53     INFO -  14606 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/dom/ranges/test_Range-insertNode.html | Finished test: Status: 2
</strong>06:16:53     INFO -  14607 INFO TEST-INFO | MEMORY STAT vsize after test: 3772563456
06:16:53     INFO -  14608 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 88804440
06:16:53     INFO -  14609 INFO TEST-END | /tests/dom/imptests/html/dom/ranges/test_Range-insertNode.html | finished in 339283ms
}

@@ +130,5 @@
> + */
> +var sortProductArray = function(a,b) {
> +  if (a == "Add-on SDK" || a == "Boot2Gecko" || a == "Core" || a == "Firefox" || a == "Firefox for Android" ||
> +    a == "Firefox for Metro" || a == "Release Engineering" || a == "Testing" || a == "Toolkit") {
> +    return -1;

Please can we sort alphabetically within each of the "top products" and "other products" (comment 33).

Also, we now need to add "Firefox OS" :-)

@@ +134,5 @@
> +    return -1;
> +  } else {
> +    if (a == "-----") {
> +      return 1;
> +    } else {

This else is redundant (see comment 32)

@@ +269,5 @@
> +    }
> +
> +    if(osstring == "Mac OS X") {
> +      platform = "x86_64";
> +    }

Let's put this and the Windows one as the initial if, and first else if, in the logic above, followed by the others as else ifs etc

@@ +323,5 @@
> +    } catch(e) { /* EAT ERRORS FOR NOW */ }
> +  }
> +  console.log(index, logstrings[index]);
> +  let failureWithContext = "Failure excerpt: \n";
> +  

nit: Trailing whitespace

::: filer/index.html
@@ +42,5 @@
> +      <div id="revisionandbranch">Revision and Branch: <span></span></div>
> +      <div id="result">Result: <span></span></div>
> +      <span id="fulllog"><a>Full log</a></span>
> +    </div>
> +    

nit: Trailing whitespace

@@ +43,5 @@
> +      <div id="result">Result: <span></span></div>
> +      <span id="fulllog"><a>Full log</a></span>
> +    </div>
> +    
> +    <h2> Select log lines to include file: </h2>

s/include file/file/

@@ +45,5 @@
> +    </div>
> +    
> +    <h2> Select log lines to include file: </h2>
> +    <div id="loglineselection"></div>
> +    

nit: Trailing whitespace
Attachment #832031 - Flags: review?(emorley) → review-
Comment on attachment 832031 [details] [diff] [review]
Next revision

Review of attachment 832031 [details] [diff] [review]:
-----------------------------------------------------------------

::: filer/filer.js
@@ +70,5 @@
> +        metadata = JSON.parse(xhr.responseText);
> +
> +        // Fetch the brief log from tbpl to get lines of context for the failures
> +        let xhr2 = new XMLHttpRequest();
> +          xhr2.open("GET", prodTBPLRoot + "php/getParsedLog.php?id=" + metadata._id + "&tree=" + metadata.branch);

nit: This line and the ones following are indented by 2 spaces too many

@@ +76,5 @@
> +            if (xhr2.readyState === 4) {
> +              if (xhr2.status === 200) {
> +                logstrings = xhr2.responseText.split("\n");
> +                console.log(logstrings);
> +                for(let i in logstrings) {

Don't forget to remove this debugging block when this lands :-)

@@ +81,5 @@
> +                  //console.log(logstrings[i]);
> +                }
> +              }
> +            }
> +        }

Missing semi-colon

@@ +131,5 @@
> +var sortProductArray = function(a,b) {
> +  if (a == "Add-on SDK" || a == "Boot2Gecko" || a == "Core" || a == "Firefox" || a == "Firefox for Android" ||
> +    a == "Firefox for Metro" || a == "Release Engineering" || a == "Testing" || a == "Toolkit") {
> +    return -1;
> +  } else {

This else also redundant

@@ +171,5 @@
> +    queries = queryString.split("&");
> +
> +    for ( i = 0, l = queries.length; i < l; i++ ) {
> +        temp = queries[i].split('=');
> +        params[temp[0]] = temp[1];

nit: 4 space indent, should be 2 :-)

@@ +305,5 @@
> +  summaryLine = summaryLine.replace("TEST-UNEXPECTED-FAIL | ", "");
> +  summaryLine = summaryLine.replace("chrome://mochitests/", "");
> +
> +  return summaryLine;
> +}

Missing semi-colon

@@ +311,5 @@
> +/*
> + *  Get some context for each submitted failure
> + */
> +var getContextForFailure = function(line) {
> +console.log("Getting context for " + line);

Presuming this is for debugging only, remember to remove before landing :-)
(Otherwise need to fix indentation)

@@ +333,5 @@
> +      window.alert("Screenshot URI omitted from summary, don't forget to attach it yourself!");
> +    }
> +  }
> +  return failureWithContext;
> +}

Missing semi-colon
Product: Webtools → Tree Management
TBPL dies in a week. Unassigning.
Assignee: wkocher → nobody
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: