Closed Bug 574189 Opened 10 years ago Closed 10 years ago

fix chrome style tests which fail when run from a .jar package

Categories

(Testing :: Mochitest, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0b5

People

(Reporter: jmaher, Assigned: jmaher)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

Attached patch fix chrome tests (v 0.1) (obsolete) — Splinter Review
in bug 543800, we are packaging all the chrome/browser-chrome/a11y tests into a .jar package.  I have found that there are 45 mochitest chrome tests and 2 browser-chrome tests that fail when run this way.
Comment on attachment 453560 [details] [diff] [review]
fix chrome tests (v 0.1)

Laurent,

Can you look at the storage tests here and see if changing the datasource from animals.sqlite to profile:animals.sqlite will achieve the same test coverage.
Attachment #453560 - Flags: feedback?(laurent)
Comment on attachment 453560 [details] [diff] [review]
fix chrome tests (v 0.1)

I didn't test but it seems good for me.

However, perhaps it could be a good thing to move the copyToProfile function to Mochitest or SimpleTest library (I don't know where exactly). It could be useful for other tests.
Attachment #453560 - Flags: feedback?(laurent) → feedback+
Attached patch fix chrome tests (v 0.2) (obsolete) — Splinter Review
updated patch to fix the toolkit/content/tests/chrome/* tests, specifically the rtltest/ and rtlchrome/ testcases.

This is not a final patch, as I plan to clean up the utility functions I am using to extract and find files in the .jar files.  

:ehsan, can you review this to assure my workaround for test_bug437844.xul and test_righttoleft.xul are actually testing the right things and you are happy with the solution.
Assignee: nobody → jmaher
Attachment #453560 - Attachment is obsolete: true
Attachment #454611 - Flags: feedback?(ehsan)
Why have you put the guts of that test in a window?  Also, I think I need to test this in a try server build, have you pushed one?
Attached patch fix chrome tests (v 0.3) (obsolete) — Splinter Review
forgot to update all changes to test_righttoleft.xul.  Kicking off a try server run, local run on desktop linux passed when these tests were failing before.
Attachment #454611 - Attachment is obsolete: true
Attachment #454611 - Flags: feedback?(ehsan)
Hmm, do the try server builds include a test package?  If not, I could also verify if these tests are testing the correct thing if you do a screen record while running the tests, and send me the video! :-)
I am going to push my .jar package and test fix patches at the same time.
Attached patch fix chrome tests (v 1.0) (obsolete) — Splinter Review
updated with a lot of tests and passing on try server.
Attachment #454620 - Attachment is obsolete: true
Attachment #460222 - Flags: review?(ctalbert)
Duplicate of this bug: 581828
Comment on attachment 460222 [details] [diff] [review]
fix chrome tests (v 1.0)

So, I did a pretty quick look through here.  I'm going to r- this version of the patch, but I think you're on the right track.

* All the harness stuff should be loaded from chrome URIs.  In some places you're adding chrome URIs to these, in some places you're removing them.  Let's ensure all harness level things should be loaded from chrome URIs.  That should reduce confusion when people are writing tests.
* Looks like all test scripts and ancilary scripts are going to be referenced by relative (non-URI) scripts, I think that's fine.  Let's make sure that is the case (though it seems to me that your patch already does this)
* Let's try to move copyToProfile out of the template extra script file and into the chrome-harness.js file.  I think it belongs there and will make things easier for test authors.  If you run into issues w.r.t. scope when you do that, then we can revisit this one.
* chrome-harness.js needs a license header since it's part of the harness.
* There are a couple of other things about chrome-harness I noticed on a quick run through.  They are below.
* In the next version of the patch, can you separate out the harness files into a separate patch from the test changes? I think that would make it easier to review.

>diff --git a/testing/mochitest/chrome-harness.js b/testing/mochitest/chrome-harness.js
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/chrome-harness.js
>@@ -0,0 +1,298 @@
>+/*
>+ * takes in a string and returns a nsIURI or nsIJARURI
>+ */
This comment is really not helpful.  A string of what?  Why would I want to use this if I were a test author?
>+function getResolvedURI(url) {
>+  var ios = Components.classes["@mozilla.org/network/io-service;1"].
>+              getService(Components.interfaces.nsIIOService);
>+  var chromeURI = ios.newURI(url, null, null);
>+
>+  var resolvedURI = Components.classes["@mozilla.org/chrome/chrome-registry;1"].
>+                    getService(Components.interfaces.nsIChromeRegistry).
>+                    convertChromeURL(chromeURI);
>+
>+  try {
>+    resolvedURI = resolvedURI.QueryInterface(Components.interfaces.nsIJARURI);
>+  } catch (ex) {} //not a jar file
>+
>+  return resolvedURI;
>+}
>+
>+function isJar(url) {
>+  var uri = getResolvedURI(url);
>+  if (uri.JARFile)
>+    return true;
>+  return false;
>+}
This function is not used anywhere in this patch (At least not that I can find).  It should be removed.

>+/**
>+ *  takes in a URI and returns the file system path where chrome points to.
>+ *  originally from browser-chrome, and for mochitest-chrome will only be called
>+ *  from getFileListing, not getJarListing.
>+ *
>+ */
Can we clean up this comment?  Is this something test authors should use?  Should not use?  

>+function getChromeDir(resolvedURI) {
>+
>+  var fileHandler = Components.classes["@mozilla.org/network/protocol;1?name=file"].
>+                    getService(Components.interfaces.nsIFileProtocolHandler);
>+  var chromeDir = fileHandler.getFileFromURLSpec(resolvedURI.spec);
>+  return chromeDir.parent.QueryInterface(Components.interfaces.nsILocalFile);
>+}
>+
>+
Nit: two blank lines ^
>+/*
>+ * given a .jar file, we get all test files located inside the archive
>+ *
>+ * basePath: base URL to determine chrome location and search for tests
>+ * testPath: passed in testPath value from command line such as: dom/tests/mochitest
>+ * dir: the test dir to append to the baseURL after getting a directory interface
>+ */
Nice comment :)
>+function getJarListing(basePath, testPath, dir)
>+{
>+  var zReader = Components.classes["@mozilla.org/libjar/zip-reader;1"].
>+                  createInstance(Components.interfaces.nsIZipReader);
>+  var fileHandler = Components.classes["@mozilla.org/network/protocol;1?name=file"].
>+                    getService(Components.interfaces.nsIFileProtocolHandler);
>+
>+  var fileName = fileHandler.getFileFromURLSpec(getResolvedURI(basePath).JARFile.spec);
>+  zReader.open(fileName);
>+  //hardcoded 'content' as that is the root dir in the .jar file
>+  var base = "content/" + dir + "/";
You'll only find this content directory if you're given a content jar file.  If you are given a locale or a skin jar, you won't find this directory.  You should probably throw an error in those cases as there is nothing that indicates this function is only for content jars. Also, I think that content folder is a convention, not a requirement.  But, since we're using it in our test jars this should be ok.  I might change the name of this function to getTestJarListing to make it more clear it's not a general-use sort of thing.

>+/*
>+ * Replication the server.js list() function with a .jar file
"Replicate"

>+//params is only used for params.testPath, gconfig.testPath in browser-chrome
>+/**
>+ * basePath: the URL base path to search from such as chrome://mochikit/content/a11y
>+ * testPath: the optional testPath passed into the test such as dom/tests/mochitest
>+ * dir: the test dir to append to the uri after getting a directory interface
>+ * srvScope: loaded javascript to server.js so we have aComponents.classesess to the list() function
>+ *
>+ * return value:
>+ *  single test: [json object, path to test]
>+ *  list of tests: [json object, null] <- directory [heirarchy]
>+ */
Combine these comments, and I'm not exactly sure what params is referring to.

>+
>+//used by tests to determine their directory based off window.location.path
>+function getRootDirectory(path) {
>+  var ios = Components.classes["@mozilla.org/network/io-service;1"].
>+              getService(Components.interfaces.nsIIOService);
>+  var chromeURI = ios.newURI(path, null, null);
>+  var myURL = chromeURI.QueryInterface(Components.interfaces.nsIURL);
>+
>+  return chromeURI.prePath + myURL.directory;
>+}
>+
>+//used by tests to determine their directory based off window.location.path
>+function getChromePrePath(path, chromeURI) {
>+
>+  if (chromeURI === undefined) {
>+    var ios = Components.classes["@mozilla.org/network/io-service;1"].
>+                getService(Components.interfaces.nsIIOService);
>+    var chromeURI = ios.newURI(path, null, null);
>+  }
>+  var myURL = chromeURI.QueryInterface(Components.interfaces.nsIURL);
>+
>+  return chromeURI.prePath;
>+}
There is only one line of difference in these two functions.  Do they need to be two functions?  How is "rootDirectory" different from "prePath"?  How would a test author know which one to use?  Can we put these together and optionally return one or the other based on a passed in param so as to avoid duplicated code?
Attachment #460222 - Flags: review?(ctalbert) → review-
Attached patch fix chrome tests (v 2.0) (obsolete) — Splinter Review
updated with review feedback.  Thanks for all the great suggestions!  Tested locally, currently running on try server.
Attachment #460222 - Attachment is obsolete: true
Attachment #461424 - Flags: review?(ctalbert)
Attached patch fix chrome tests (v 2.1) (obsolete) — Splinter Review
updated for bitrot in 1 file.
Attachment #461424 - Attachment is obsolete: true
Attachment #462293 - Flags: review?(ctalbert)
Attachment #461424 - Flags: review?(ctalbert)
Comment on attachment 462293 [details] [diff] [review]
fix chrome tests (v 2.1)

ctalbert: jmaher: I thought you were going to change all the references of mochitest harness stuff to chrome:// paths?
[7:12pm] ctalbert: and all things not harness stuff was not going to be loaded with relative paths
[7:12pm] ctalbert: er and all things not harness stuff was not going to be loaded with relative paths
[7:13pm] jmaher: ctalbert: yeah
[7:13pm] ctalbert: bear, huh, have to check on that one then.  We basically kick off the harness and let it go, so it's likely something in that test interacting poorly with either the standard reftest harness in chrome or that bit not working with fennec
[7:14pm] jmaher: that patch should be exactly what you said
[7:14pm] ctalbert: jmaher: look at the diff on that bug -- third diff down, test_draggableprop.html - exactly the opposite
[7:14pm] ctalbert: you're changing mochitest harness stuff to use local paths
[7:14pm] ctalbert: er relative paths
[7:14pm] ctalbert: damn but I can't type 
[7:16pm] ctalbert: jmaher: since we wanted to keep the chrome paths for mochitest harness stuff, that seems like unnecessary change
[7:17pm] jmaher: ctalbert: I removed the chrome:// from mochitest-plain
[7:17pm] • ctalbert looks again
[7:17pm] jmaher: because that requires us to have the mochikit.* installed
[7:18pm] ctalbert: oh shit those are mochitest-plain tests
[7:18pm] jmaher: so those tests that have it removed are just mochitest-plain
[7:18pm] jmaher: sorry, I should have been a bit more clear
[7:18pm] ctalbert: ok, cool
[7:18pm] ctalbert: very very good, then.  well done jmaher 
[7:18pm] jmaher: heh, thanks!
[7:18pm] • ctalbert starts again at the top
[7:19pm] • jmaher passed the first quest
[7:19pm] • ctalbert was somehow thinking all the tests in this patch were chrome tests. 
[7:21pm] ctalbert: jmaher: if the stuff in content/xul/templates/tests/chrome/templates_shared.js throws, then the test is going to fail, correct?
[7:21pm] jmaher: heh, this patch cleans up a lot of stuff
[7:22pm] ctalbert: i.e. if copyToProfile throws, then the test is going to fail as it won't have its files available
[7:22pm] jmaher: correct
[7:22pm] ctalbert: So, can we do an ok(false, "Cannot copy files to profile due to: " + ex); in that catch statement just to aid in debugging later (when they change e10s again)
[7:23pm] ctalbert:
[7:23pm] jmaher: yeah!
[7:23pm] jmaher: good idea
[7:28pm] ctalbert: I'd check dom/tests/mochitest/whatwg/test_postMessage_chrome.html -- looks like you're removing the beginning of a comment but not the end of it?
[7:29pm] ctalbert: not sure why you're removing the comment in teh first place, but that coudl be  a bugzilla viewer bug
[7:33pm] jmaher: ctalbert: hmm, the diff is a bit odd, but yeah
[7:34pm] jmaher: the diff is --->, where --> is the termination of the comment and the extra - is the diff minus
[7:35pm] ctalbert: ah, I figured it was soemthing like that, which probably only appears in the raw view of the diff
[7:35pm] jmaher: why the comment is removed?  I believe I did it because it was either a mochikit.jar!/path, or a relative path...not what the old comment used to be
[7:37pm] ctalbert: I figured, ok
[7:37pm] ctalbert: I was thinking it would still be served from chrome, but yet the exact url would have to change coming from a jar
[7:39pm] ctalbert: jmaher: one more -- down a ways in test_syntesizeDrop.xul - you remove utils.js?  Not sure what's in there...going to look that up
[7:39pm] ctalbert: I see why, it's one of those horrible cross-links
[7:40pm] • ctalbert just wonders how the test keeps functioning
[7:40pm] jmaher: ctalbert: I don't believe it was being used
[7:40pm] jmaher: there is still one more test in the tree that cross references a library
[7:40pm] jmaher: but in this case it was included but not used
[7:40pm] ctalbert: jmaher: awesome catch
[7:42pm] jmaher: ctalbert: good catch on your part with the detailed review
[7:43pm] ctalbert: haha, I try
[7:44pm] ctalbert: jmaher: ok, last one review comment - in chrome-harness.js - it's no longer 2008, so update your license header.
[7:45pm] • jmaher is living in the past
[7:45pm] ctalbert: r+ with those changes
Attachment #462293 - Flags: review?(ctalbert) → review+
Attached patch fix chrome tests (v 2.2) (obsolete) — Splinter Review
updated with nit's from r+
Attachment #462293 - Attachment is obsolete: true
Attachment #463043 - Flags: review+
Joel, I watched the tree all day moving from half/closed to burning to closed, and was unable to land this for you. I'm setting to checkin-needed, perhaps you can get Ted or Jgriffin to land this for you next week.  Sorry about that.
Keywords: checkin-needed
Landed: http://hg.mozilla.org/mozilla-central/rev/38cc0d7d6667
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Slight update to error messages for copyToProfile where it would spew errors when the file existed (if a second test copied to the same location).  This was causing a lot of errors on windows runs and I had it locally but not on this patch.

This passed twice on try server over the weekend.
Attachment #463043 - Attachment is obsolete: true
this was backed out shortly thereafter as a result of a series of failures with the whole set of patches that landed with it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #17)
> Created attachment 468287 [details] [diff] [review]
> fix chrome tests (v 2.3)
> 
> Slight update to error messages for copyToProfile where it would spew errors
> when the file existed (if a second test copied to the same location).  This was
> causing a lot of errors on windows runs and I had it locally but not on this
> patch.
> 
> This passed twice on try server over the weekend.

Landed this version: http://hg.mozilla.org/mozilla-central/rev/a90640c20652
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 603234
Target Milestone: --- → mozilla2.0b5
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.