416049 - Minotaur - Needs better human readable output

Status: RESOLVED FIXED

(Testing Graveyard :: Minotaur, defect)

Description

[cmtalbert]

Attachment: Current output diff of test-output.xml files on a finnish build

The current tool does not output decently readable files, and sometimes the error messages can be misleading.  For example, if the tool discovers that the Build Under Test did not produce ANY bookmarks output, it reports that "bookmark lists are not the same length" which doesn't necessarily raise the same red flags as "build xxx does not have any default bookmarks".  

This bug is to address this and clean it up.  

In short the plan is to:
1. Provide a better diff output when comparing the test-output.xml files 
2. Provide more meaningful messages when encountering empty files (bookmarks, output.xml's etc)
3. Provide the same type of diff output when comparing the bookmarks files so that a person can easily analyze the results file without having to bend their mind around two different output schemes.
4. The diff output must of course handle UTF-8 encodings properly

I will attach an example of a current output log.

Comment 1

[cmtalbert]

Attachment: Full update to minotaur patch

Attaching wip patch that covers this bug, 414056, 413742, and 396131

The current output is our standard unified diff format that we use for patches.

If a bookmarks file is missing it tells you that IN BIG LETTERs.  :-)

Feedback welcome.  I'll split this into easily reviewable chunks on each bug once I finish testing it.

Comment 2

[cmtalbert]

Attachment: Patch to change the way bookmarks are reported

This patch changes the way the bookmark data is reported.  It makes several other user improvements such as better messaging when a bookmark file is missing and handling 0 args into the user runnable partner.py and run-minotaur.py scripts.

Comment 3

[cmtalbert]

Attachment: Full minotaur diff of all changes, just for reference

Comment 4

[cmtalbert]

(In reply to comment #2)
> Created an attachment (id=303640) [details]
This also addresses bug 396131

Comment 5

[alice nodelman [:alice] [:anode]]

I'm seeing some overlap between this patch and that which I just reviewed for bug 413742 (in run-minotaur.py).  Can I get a fresh diff that only has the changes for this bug?  Unless I've just got myself all confused...

Comment 6

[cmtalbert]

Yes it does contain some similar changes.  I tried to mitigate as best I could, but it wasn't that possible.  Let me get you another patch.

Comment 7

[cmtalbert]

Attachment: Updated Patch since bug 413742 is fixed

The new patch you requested

Comment 8

[alice nodelman [:alice] [:anode]]

Comment on attachment 304613 [details] [diff] [review]
Updated Patch since bug 413742 is fixed

I'm not a big fan of this being a hard coded value instead of a command line switch:

+DIFFBKMK_DBG = 0
+
+def debug(s):
+  if DIFFBKMK_DBG:
+    print s
+

Also, this help message doesn't really indicate what went wrong or what the correct format would look like.

+  # Print Help if no args passed
+  if not options.branch or not options.version or not options.url or \
+     not options.l10nFile or not options.partner or not options.minDir:
+    print "Required Items Not Specified. Must specify partner name, minotaur dir,",
+    print "locale file, branch, url, and version"
+    parser.print_help()
+    sys.exit(1)

Comment 9

[cmtalbert]

(In reply to comment #8)
> (From update of attachment 304613 [details] [diff] [review])
> I'm not a big fan of this being a hard coded value instead of a command line
> switch:
> 
> +DIFFBKMK_DBG = 0
> +
> +def debug(s):
> +  if DIFFBKMK_DBG:
> +    print s
> +
> 
Good point I'll enable this to be turned on by a command line input

> Also, this help message doesn't really indicate what went wrong or what the
> correct format would look like.
> 
> +  # Print Help if no args passed
> +  if not options.branch or not options.version or not options.url or \
> +     not options.l10nFile or not options.partner or not options.minDir:
> +    print "Required Items Not Specified. Must specify partner name, minotaur
> dir,",
> +    print "locale file, branch, url, and version"
> +    parser.print_help()
> +    sys.exit(1)
> 
Actually, it does. parser.print_help uses the help and metaVar and the parameter name values in the add_option calls to print out a very nice help view.  Here is an example of what is printed from the command python partner.py

Required Items Not Specified. Must specify partner name, minotaur dir, locale file, branch, url, and version
Usage: partner.py [options]

Options:
  -h, --help            show this help message and exit
  -p PARTNER_NAME, --Partner=PARTNER_NAME
                        Partner Name
  -b BRANCH, --Branch=BRANCH
                        Gecko Branch: 1.8.0|1.8.1|1.9
  -v FIREFOX_VERSION, --Version =FIREFOX_VERSION
                        version of firefox to be tested
  -u URL, --UrlToBuild=URL
                        URL to top level build location, above the OS
                        directories
  -f VER_FILES, --VerificationFileLocation=VER_FILES
                        location of verification files, leave blank to create
                        verification files
  -m MINOTAUR_DIR, --MinotaurDirectory=MINOTAUR_DIR
                        Directory of the Minotuar code
  -e EXT_NAME, --ExtensionName=EXT_NAME
                        Name of the partner extension.  Only needed if Partner
                        has EULA
  -c CREDENTIALS, --Credentials=CREDENTIALS
                        Credentials to download the build in this form:
                        <user>:<password>
  -a AUS_PARAM, --AusParameter=AUS_PARAM
                        The AUS parameter for the AUS URI (-cck param)
  -l L10N_FILE, --L10NFile=L10N_FILE
                        A text file                    containing the language
                        codes for this build, separated                    by
                        LF

The only issue I see with this help output is that the L10N file file is a bit messed up.  I'll correct that in the next version of the patch.

Comment 10

[cmtalbert]

Attachment: Patch addressing comments

New patch that allows user to specify -d <something> on the command line to turn on debug logging.  Changed checkBookmarks so that it can inherit the debug setting since most of the useful debug output comes from that class.  Also cleaned up the help display in partner.py

Comment 11

[alice nodelman [:alice] [:anode]]

Comment on attachment 306565 [details] [diff] [review]
Patch addressing comments

There is still a hardcoded debug value in partner.py (isDebug = True).  I think that it might be worth it to have another look at how the debug value is set and passed to the various modules, but that can be split off to another bug.

Comment 12

[cmtalbert]

(In reply to comment #11)
> (From update of attachment 306565 [details] [diff] [review])
> There is still a hardcoded debug value in partner.py (isDebug = True).  I think
> that it might be worth it to have another look at how the debug value is set
> and passed to the various modules, but that can be split off to another bug.
> 
Opened Bug 420965 for the hard coded debug value issue, and to look into using the standard python logging module.

Checked in patch on trunk --> Fixed

Comment 13

[(not currently active) Ted Mielczarek]

Mass moving Minotaur bugs to Testing : Minotaur. Filter on MinotaurMassMove to ignore.