534744 - Software update tests should be able to send detailed update reports to brasstacks

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We are able to send reports to couchdb now. Only support for restart tests is missing. That will be done in bug 533227.

With that implementation the software update tests should be able to send reports to brasstacks. The question is if they should be part of the normal mozmill results or if we should track those in a separate couchdb database.

Would be great if we could have all the tests in one db but if that makes things harder lets go with another database. Mikeal, any objections?

Comment 1

[Mikeal Rogers [:mikeal] (mrogers@)]

Just finished breaking up all this logic so you can do this.

pull from mikeal/mozmill/tree/reports and it should be clear how to go about
implementing this by looking at my code for the restart tests.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mikeal, I will check the reporting stuff shortly. So I will report back on bug 533227.

For this particular feature would you prefer to use another database for the reports?

Comment 3

[Mikeal Rogers [:mikeal] (mrogers@)]

Go ahead and stick em all in brasstacks, should be fine.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

But we need different views for presentation. Given that we should also add a report type when sending those data. We do not wanna have mixed output for normal Mozmill tests and software update tests.

Comment 5

[Mikeal Rogers [:mikeal] (mrogers@)]

just create a new design document for the new views and check the document type before emitting.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alright. Given that information I would also have to update the currently available views so they only retrieve information from Mozmill tests and do not mix content from Software Update tests. For the latter ones I will use "SoftwareUpdate" as document type. Have to wait for bug 533227...

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This no longer blocks the 1.4.2 release of Mozmill. Sub classes can perfectly modify and add their own data to the report.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With the patch on bug 604370 we have all the information to start with the update of the automation script.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This patch makes sure that we send all the collected meta data from one or multiple updates up to brasstacks. Further it constructs a wiki like output from data of the first and last update.

Anthony, would be great if you can test it again. Now with this patch applied to the /data/scripts folder.

Comment 10

[Jeff Hammel]

Comment on attachment 491492 [details] [diff] [review]
Patch v1

>+        updates = result.get("updates")

Why not result['updates'] if you're not going to error check anyway?

>+        first_update = updates[0]
>+        last_update = updates[len(updates) - 1]
>+

should be `last_update = updates[-1]`

>         entry = "* %s => %s, %s, %s, %s, %s, %s, '''%s'''\n" \
>                 "** %s ID:%s\n** %s ID:%s\n" \
>                 "** Passed %d :: Failed %d :: Skipped %d\n" % \
>-                (results.get("preVersion", ""),
>-                 results.get("postVersion", ""),
>-                 results.get("type"),
>-                 results.get("preLocale", ""),
>-                 results.get("updateType", "unknown type"),
>-                 results.get("channel", ""),
>+                (first_update.get("build_pre").get("version"),
>+                 last_update.get("build_post").get("version"),
>+                 last_update.get("patch").get("type"),
>+                 first_update.get("build_pre").get("locale"),

Are these dicts?  Why not use e.g. first_update['build_pre']['locale'] ?  dict.get is mostly used for working around missing keys, but if e.g. 'build_pre' not in first_update anyway this will break

>+                 "complete" if last_update.get("patch").get("is_complete") else "partial",

I'd prefer to separate out this logic;  Its probably also better to use a string template that references the variables, as its pretty much unreadable as is.  Likewise for the subsequent logic

Comment 11

[u279076]

(In reply to comment #9)
> Created attachment 491492 [details] [diff] [review]
> Patch v1

RE: feedback?
Should I wait for an r+ patch before testing and giving feedback?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Fixed all the review comments except for the template one, because this is only a temporary solution for the next 3-4 weeks.

Comment 13

[Jeff Hammel]

Comment on attachment 491625 [details] [diff] [review]
Patch v2

wfm

Comment 14

[u279076]

Attachment: Testrun Log

Here is a log of a testrun using this patch.

COMMAND:
/data/scripts/testrun_update.py --repository=/data/testing/ashughes/mozmill-tests --report=file:///data/testing/ashughes/testrun.log --channel=beta /data/testing/4.0/update_mac/firefox-4.0b6.en-US.dmg

WIKI RESULTS:
* 4.0b6 => 4.0b7, minor, en-US, complete, beta, 2010-11-18, '''PASS'''
** Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 ID:20100914072643
** Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7) Gecko/20100101 Firefox/4.0b7 ID:20101104131842
** Passed 5 :: Failed 0 :: Skipped 0

* 4.0b6 => 4.0b6, minor, en-US, complete, beta, 2010-11-18, '''FAIL'''
** Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 ID:20100914072643
** Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 ID:20100914072643
** Passed 6 :: Failed 2 :: Skipped 0

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Move of Mozmill related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v3

I have spotted another missing piece inside the wiki output. We miss to combine the two results for direct and fallback updates because we still access the old members. We also have to output the fallback string.

Comment 17

[Jeff Hammel]

Comment on attachment 493821 [details] [diff] [review]
Patch v3

>+    def build_wiki_entry(self, result):
>+        """ Until we show results on the dashboard create a wiki like output
>+            format for data from the first and last update performed """

docstrings usually don't have leading and trailing spaces

>+        entry = "* %s => %s, %s, %s, %s%s, %s, %s, '''%s'''\n" \
>                 "** %s ID:%s\n** %s ID:%s\n" \
>                 "** Passed %d :: Failed %d :: Skipped %d\n" % \

As unreadable as this is without using """* %(preVersion)s => % (postVersion)s ...""", its probably a little better to use triple quotes to make it slightly more readable

>+                (first_update["build_pre"]["version"],
>+                 last_update["build_post"]["version"],

So are these values guaranteed to be there? Will there ever be a case where they will fail because they're not there

>+                 "complete" if last_update["patch"]["is_complete"] else "partial",
>+                 "+fallback" if last_update["fallback"] else "",

It would be better to keep logic out of this entirely.  I'm not really sure why its important to have this now and what it will be replaced with, but this is a lot of logic for a single string interpolation.  Its at least better to fill out a dict first and then interpolate with that rather than building all the logic directly into the interpolation.  It makes it harder to read and debug.

I also thing the <foo> if <condition> else <bar> is not available in python 2.4.  Not sure if that's an issue?

I didn't count the number of %s versus the number of fields.  I'm going to assume that you can verify that they're identical

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #17)
> docstrings usually don't have leading and trailing spaces

Thanks I will put this onto my list for the rewrite of the module. Nearly all the doc strings in this file have to be updated. It's a bit too much for this patch.

> >+        entry = "* %s => %s, %s, %s, %s%s, %s, %s, '''%s'''\n" \
> >                 "** %s ID:%s\n** %s ID:%s\n" \
> >                 "** Passed %d :: Failed %d :: Skipped %d\n" % \
> 
> As unreadable as this is without using """* %(preVersion)s => % (postVersion)s
> ...""", its probably a little better to use triple quotes to make it slightly
> more readable

Thanks! Haven't known about this feature. I will take care of it in the future. It makes sense. But this complete function will go away soon.

> >+                (first_update["build_pre"]["version"],
> >+                 last_update["build_post"]["version"],
> 
> So are these values guaranteed to be there? Will there ever be a case where
> they will fail because they're not there

They will always be there, yes.

> >+                 "complete" if last_update["patch"]["is_complete"] else "partial",
> >+                 "+fallback" if last_update["fallback"] else "",
> 
> It would be better to keep logic out of this entirely.  I'm not really sure why
> its important to have this now and what it will be replaced with, but this is 

The handling was part of the update tests itself. But that has been moved out to the python side with my patches on bug 604370. There is no reason why tests should handle that.

Thanks Jeff.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/1f4778986d18