Closed Bug 385987 Opened 17 years ago Closed 17 years ago

Minotaur Testing Tool for Partner Builds and L10N Testing

Categories

(Testing Graveyard :: Minotaur, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: cmtalbert)

References

()

Details

Attachments

(2 files, 7 obsolete files)

Attached patch Minotaur Tool -- rev1 (obsolete) — Splinter Review
This is the Minotaur test tool for Partner Builds and L10N Testing.  This test tool will hopefully automate much of the work that is currently done by hand to check search order, search terms, preferences, extensions, and bookmark configurations for these builds.  Please see the quality.mozilla.org URL above for more information on it.

The attached patch is a first pass at the implementation.  There are some notes:
1. Most importantly: It needs to be more flexible w.r.t. profile paths.  As you can easily see in minotaur.sh these are hard-coded.  What troubles me is that the CreateProfile switch on Firefox seems to want something more native than what we need elsewhere at least for Windows.  I don't expect this to be a problem for other platforms, but I may need to write a utility to handle that for Windows.
2. Now, minotaur.sh is the only startup script that the tool needs.  It's parameter list should *NOT* be considered final. It takes the name of the build to test (just a user defined name), the locale of the build, and the installed location of firefox as a path).  
3. This still has only been run on Windows.  I will be expanding it now to check that it runs on other systems.  I don't expect to encounter any issues outside of the issues w.r.t. the shell file.  Is there a non-cygwin method that we can use to determine what OS we are on so that the script can be properly "#ifdef'd"?
4. Output files: It currently outputs two files, a textual log so that a QA person can tell what went wrong with a test run (or myself, in a debugging role). It also outputs a simple snippet of HTML that I envision might be cherrypicked with other test results at some point to create a visual display of a multiple build/locale test run.
Attachment #269932 - Flags: review?(rcampbell)
Attached patch Minotaur Tool -- rev2 (obsolete) — Splinter Review
Attachment #269932 - Attachment is obsolete: true
Attachment #269936 - Flags: review?(rcampbell)
Attachment #269932 - Flags: review?(rcampbell)
this looks like a great start, Clint. Nice work. One recommendation, you should break up long lines as much as possible. Browsing code in lxr/bonsai gets a little funny with lines longer than 72 characters (I know, I know...).

Great stuff.
Attached patch Minotaur -- rev3 (in progress) (obsolete) — Splinter Review
This is an in progress snapshot of the code as I move toward reading the search link/search param information directly from the Verification file and replacing the regexp based verification that had been done for those previously.

I also consolidated the verification process into an object.  Note the new code is in checkSearch.py, the old checkSearchLinks.py is still included in the patch for reference, but will be removed before checkin.
Attachment #269936 - Attachment is obsolete: true
Attachment #269936 - Flags: review?(rcampbell)
Comment on attachment 272206 [details] [diff] [review]
Minotaur -- rev3 (in progress)

could you rediff with cvs diff -u8pN please? Without context it's a little hard to see what's being added to where. Thanks.
Attachment #272206 - Flags: review-
Rob, AFAICT, that's all new.

I have one nit, the 
#!/usr/bin/env python
lines in the middle of real python modules are useless.

And at some point, reducing the amount of required additional python modules would make it easier to deploy the tests, in particular as the requirements are likely going to be on the buildbot slave side.
oops, new code it is, sorry about that. I can second Axel's suggestion to remove nested module #! lines though. No point in having them if they're not being executed.

Also, removal of dependencies is nice to have if possible. If these prerequisite modules are absolutely necessary then we can include the dependency, but they should be avoided if at all possible.

To add one nit of my own, I'd like to see the commented out sections of code removed before final patch. Looks promising though!
Attached patch Revision 4 of Minotaur code (obsolete) — Splinter Review
Thanks for the reviews, guys.  Here is a patch that addresses these review comments and removes the library dependencies.  I also cleaned up the last of the "long lines" issues were they were absolutely egregious as well.
Attachment #272206 - Attachment is obsolete: true
Attachment #272738 - Flags: review?(rcampbell)
I have glanced at a three files, and I found more, and I expect my review comments to be longish :-(, so I'm wondering how we wanna do this. I have some general style comments which I can shoot first:

if ():, the braces are not very python-ish.

I'm probably the odd one out on this, but I prefer 2 space indention to 4 space. I guess 4 is standard outside the Mozilla world, though we use 2 for most stuff now, AFAICT.

I have a bunch of code comments, should we address those in a second batch after an initial landing, or should I paste them per file, or as one huge comment?

Overall, I guess we won't be able to switch minotaur on before the absolute paths are not removed from the code, too.
(In reply to comment #8)
> I have glanced at a three files, and I found more, and I expect my review
> comments to be longish :-(, so I'm wondering how we wanna do this. I have some
> general style comments which I can shoot first:
> 
Axel, I'd prefer to have all the comments up front so that I can just work through them more quickly, so if you'll type up your comments I'll be happy to implement them.

> if ():, the braces are not very python-ish.
Ah, a hold over from all my C++ experience.  I'll be happy to remove them.
 
> I'm probably the odd one out on this, but I prefer 2 space indention to 4
> space. I guess 4 is standard outside the Mozilla world, though we use 2 for
> most stuff now, AFAICT.
I didn't realize that we had switched to 2 space indenting across the board.  The original minotaur code used 4 space, so that was what I continued to use.  I'll be happy to change this to follow the style already used by the other testing/ components 

> I have a bunch of code comments, should we address those in a second batch
> after an initial landing, or should I paste them per file, or as one huge
> comment?
Please go ahead and make them and I'll update the code, as I mentioned above.
 
> Overall, I guess we won't be able to switch minotaur on before the absolute
> paths are not removed from the code, too.
There is an absolute path in testVerify.xml which is not part of Minotaur per se.  That is a hand-generated verification xml file that is only checked in so that we have a reference format for those files.  We will be automatically generating these verification XML files for each build under test when Minotaur actually goes live.  That icon attribute on extensions is not currently checked by the current code (I haven't found anything in the partner builds that require it to be checked).

The other locations that have absolute paths are in test.manifest and minotaur.sh which are the boot-strap files that allow minotaur to be run from the shell.  Minotaur.sh will be replaced by a build-bot specific bootstrapping mechanism that will handle the pathing issues smartly.  The tests.manifest file will be parametrized so that the proper path can be dropped in by the build-bot specific configuration scripts.

Are there other absolute paths that you're concerned about?
Judging from our call, let me add a few more comments:

testing/release/minotaur/checkBookmarks.py

Could you group the sgml callbacks that they somehow flow along the bookmarks.html structure?

INITIAL, IN_H3, IN_A = range(3)

seems to be the standard way to set state vars. You could put them globally, or in the class.

class F:
   A,B,C = range(3)
   def __init__(self):
     pass

is a sample.

+    for a in attributes:

call that var attr? a is a link everywhere else.

+    bkmkList.sort(tupleComparator)

bkmkList.sort(lambda x,y: cmp(x[0], y[0]))

should work just as well.

checkSearch.py:

I found termDict.has_key(p), this might be elsewhere, too. has_key is frowned upon, or at least, so I'm told.

if p in termDict:
  this makes python smile.

I guess the easiest way to enhance the search test to actually test for set and order independently is to allow <searchlink> to just not have an order.

The bracing in quit.js look non-standard with opening { in their own line. Same in other js files.

runTests.py:

I prefer optparse to getopt. It gives you good usage strings by default, too. The actual parsing of the command line should be done in 
if __name__ == "__main__":
and the main() function should be a properly named function with proper function arguments, if feasible with good defaults. If you ever happen to call into that function considering runTests.py a module, you wouldn't want to fake sys.args.

testing/release/minotaur/sb.js
+  output ("\n<section id=\"preferences\">\n");

no ' ' between function and arguments.

Not sure why you get the RDF service into RDF2, you should just use RDF if it exists. (Or is it not the RDF service in the end?)

That'd be the things I found by glancing over all files, I didn't really check each individual line.

Regarding the search tests, according to Basil we should test the set of search engines and their functionality, and for those with explicit ordering, that, too. That is, if we happen to have missed, say, removing answers.com, the error log should actually say "answers is still there", and not give false positives on the ebay plugin coming after that. Getting this right is likely some joint work of Clint and Mic to make the spec actually give the information in a way that makes this easy.
Comment on attachment 272738 [details] [diff] [review]
Revision 4 of Minotaur code

Cancelling review, working on new patch
Attachment #272738 - Flags: review?(rcampbell)
Attached patch Minotaur rev 5 (obsolete) — Splinter Review
I believe this patch should hopefully pass muster.  I have addressed all of Axel's comments, and have added update channel checking for partner builds.

I also worked out several odd bugs in testing.  I think this can serve as a first draft and we can check this in.  There are of course some rough spots that still need to be worked out, but might be best addressed in follow-on bugs:

1. Verification files - there are three verification files being checked in with this bug. They have the naming convention <partner>-<locale>-verify.xml.  We need to figure out the proper naming convention (should include version too) and their final resting place.  They probably should belong here in CVS, but maybe in their own directory: testing/release/minotaur/verification?

2. There are certain preferences that I am experimenting with grabbing in order to check for feed handler registrations.  If these prefs are not specified in the verification file, then Minotaur will mark that as a failure.  I'm trying to decide about whether or not this is actually a failure.  Elsewhere, I've used the logic that "if it isn't in the verification file, then it isn't a failure."  This is a nice way to think about it because then the verification file dictates exactly what we want to see in the build under test, no questions, no ambiguities.

3. And the corrollary from point 2.  These verification files are extremely important.  And they have to be correct.  I need to come up with an automated or semi-automated mechanism for writing them.  For L10N, that's pretty easy.  For partner builds, even with the DEX ini file, that's not going to be so easy.

4. Clean up.  Currently all the cleanup logic is commented out.  This is useful for debugging, and will need to be fixed once we integrate this with buildbot and install the tool on its final home set of boxes.

5. And of course, I need to do the integration work, mentioned in step 4.
Attachment #272738 - Attachment is obsolete: true
Attachment #276232 - Flags: review?(rcampbell)
(In reply to comment #10)
> Judging from our call, let me add a few more comments:
> 
Sorry I didn't respond to these sooner....

> Could you group the sgml callbacks that they somehow flow along the
> bookmarks.html structure?

done

> seems to be the standard way to set state vars. You could put them globally, or
> in the class.
done, thanks

> call that var attr? a is a link everywhere else.
done

> 
> +    bkmkList.sort(tupleComparator)
> 
> bkmkList.sort(lambda x,y: cmp(x[0], y[0]))
done, thanks
> I found termDict.has_key(p), this might be elsewhere, too. has_key is frowned
> upon, or at least, so I'm told.
> 
> if p in termDict:
>   this makes python smile.
python now smiles :-)

> The bracing in quit.js look non-standard with opening { in their own line. Same
> in other js files.
fixed
> 
> I prefer optparse to getopt. 
good points.  Refactored.

> testing/release/minotaur/sb.js
> +  output ("\n<section id=\"preferences\">\n");
> 
> no ' ' between function and arguments.
fixed

> Not sure why you get the RDF service into RDF2, you should just use RDF if it
> exists. (Or is it not the RDF service in the end?)
Can't use RDF.  Had to re-instantiate it.  I think RDF2 was just a typo.
(fixed)

> Regarding the search tests, according to Basil we should test the set of search
> engines and their functionality, and for those with explicit ordering, that,
> too. That is, if we happen to have missed, say, removing answers.com, the error
> log should actually say "answers is still there", and not give false positives
> on the ebay plugin coming after that. Getting this right is likely some joint
> work of Clint and Mic to make the spec actually give the information in a way
> that makes this easy.
I think this is going to have to be addressed in the way that we create the verification files.  The "exporter" part of minotaur only exports the data as it exists in the application.  The "verification" part of minotaur only vets the exported data as per the verification file.  We've removed all knowledge that was hard coded into the system.  I hope this addresses your comments, if not we can talk about it next week. 

Status: NEW → ASSIGNED
Clint, should we get a new patch after our discussion to cut down on the scope of the verification step?
Depends on: 393070
Comment on attachment 276232 [details] [diff] [review]
Minotaur rev 5

Yes, it is in progress, probably up here tomorrow.  For now, I'll cancel that stale review request.
Attachment #276232 - Flags: review?(rcampbell)
Attached patch Minotaur Final (obsolete) — Splinter Review
Here is the final patch.  The windows specific shell file has disappeared.  Now, there is only one file and I use a python script to detect the OS and set OS specific settings properly. This patch contains the "second approach" that was discussed at the mozilla all hands and uses the diffs with previous versions of minotaur output to detect changes to those files.  

== Similarities with the previous patches ==
The new diffBookmarks.py merely uses the unchanged code from checkBookmarks.py (the parser) in a new way to perform a bookmarks.html diff.  The checkReleaseChannel.py is new, but is copied code from the old checkHttpLog.py.  I needed different entry points there, so I just re-wrote it.  Likewise, the LogWriter class of old morphed (nearly unchanged) into LogAppender.py to write out a simple log file of any encountered failures.
Attachment #276232 - Attachment is obsolete: true
Attachment #279073 - Flags: review?(rcampbell)
Readme file for the Minotaur tool with instructions on how to set it up and some known issues that folks are likely to run into.
This patch addresses Rob's concerns with the Minotaur.sh file.
* It now has a -m parameter that allows the user to specify the Minotaur Directory
* It now will detect the generation of verification files and will display appropriate messaging.

Still no clue on the random failure Rob saw when he first attempted the patch.
Attachment #280416 - Flags: review?(rcampbell)
Attachment #279073 - Attachment is obsolete: true
Attachment #280416 - Attachment is obsolete: true
Attachment #280757 - Flags: review?(rcampbell)
Attachment #279073 - Flags: review?(rcampbell)
Attachment #280416 - Flags: review?(rcampbell)
Could you guys get the review comments off the hidden track?

At one point soon, I'd like to look at this again, and knowing what is supposed to be changed would definitely help.
Comment on attachment 280757 [details] [diff] [review]
Updated patch with review comments

couple of minor tweaks: #! line in minotaur.sh should probably point to /bin/bash instead of /usr/bin. Increase timeout on line 97 to 10s to cover slow systems like mine. Icons in test-bookmarks.html are not checked, but that can be handled in a follow-up.
Attachment #280757 - Flags: review?(rcampbell) → review+
(In reply to comment #21)
> (From update of attachment 280757 [details] [diff] [review])
> couple of minor tweaks: #! line in minotaur.sh should probably point to
> /bin/bash instead of /usr/bin. Increase timeout on line 97 to 10s to cover slow
> systems like mine. Icons in test-bookmarks.html are not checked, but that can
> be handled in a follow-up.
> 
Bug 396131 filed for the icon bug, nits addressed.
Bug 396132 also filed to update Minotaur's vetting of favicons in the bookmarks files.

Check in log
h-144:~/code/fx-trunk/mozilla clint$ cvs commit -m "Bug 385987 - initial check in of minotaur tool r=robcee" testing/release/minotaur
cvs commit: Examining testing/release/minotaur
? testing/release/minotaur/README.txt
RCS file: /cvsroot/mozilla/testing/release/minotaur/checkBookmarks.py,v
done
Checking in testing/release/minotaur/checkBookmarks.py;
/cvsroot/mozilla/testing/release/minotaur/checkBookmarks.py,v  <--  checkBookmarks.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/checkReleaseChannel.py,v
done
Checking in testing/release/minotaur/checkReleaseChannel.py;
/cvsroot/mozilla/testing/release/minotaur/checkReleaseChannel.py,v  <--  checkReleaseChannel.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/diffBookmarks.py,v
done
Checking in testing/release/minotaur/diffBookmarks.py;
/cvsroot/mozilla/testing/release/minotaur/diffBookmarks.py,v  <--  diffBookmarks.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/getOsInfo.py,v
done
Checking in testing/release/minotaur/getOsInfo.py;
/cvsroot/mozilla/testing/release/minotaur/getOsInfo.py,v  <--  getOsInfo.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/logAppender.py,v
done
Checking in testing/release/minotaur/logAppender.py;
/cvsroot/mozilla/testing/release/minotaur/logAppender.py,v  <--  logAppender.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/minotaur.sh,v
done
Checking in testing/release/minotaur/minotaur.sh;
/cvsroot/mozilla/testing/release/minotaur/minotaur.sh,v  <--  minotaur.sh
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/minotaur.xul,v
done
Checking in testing/release/minotaur/minotaur.xul;
/cvsroot/mozilla/testing/release/minotaur/minotaur.xul,v  <--  minotaur.xul
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/quit.js,v
done
Checking in testing/release/minotaur/quit.js;
/cvsroot/mozilla/testing/release/minotaur/quit.js,v  <--  quit.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/quit.xul,v
done
Checking in testing/release/minotaur/quit.xul;
/cvsroot/mozilla/testing/release/minotaur/quit.xul,v  <--  quit.xul
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/sb.js,v
done
Checking in testing/release/minotaur/sb.js;
/cvsroot/mozilla/testing/release/minotaur/sb.js,v  <--  sb.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/tests.manifest,v
done
Checking in testing/release/minotaur/tests.manifest;
/cvsroot/mozilla/testing/release/minotaur/tests.manifest,v  <--  tests.manifest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/testing/release/minotaur/user.js,v
done
Checking in testing/release/minotaur/user.js;
/cvsroot/mozilla/testing/release/minotaur/user.js,v  <--  user.js
initial revision: 1.1
done
h-144:~/code/fx-trunk/mozilla clint$ cvs commit -m "Bug 385987 Minotaur Tool initial check in, r=robcee" testing/release/minotaur/README.txt
RCS file: /cvsroot/mozilla/testing/release/minotaur/README.txt,v
done
Checking in testing/release/minotaur/README.txt;
/cvsroot/mozilla/testing/release/minotaur/README.txt,v  <--  README.txt
initial revision: 1.1
done



Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Mass moving Minotaur bugs to Testing : Minotaur. Filter on MinotaurMassMove to ignore.
Component: Testing → Minotaur
Product: Core → Testing
QA Contact: testing → minotaur
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: