package chrome style tests into .jar file for local testing

RESOLVED FIXED in mozilla2.0b7

Status

defect
RESOLVED FIXED
9 years ago
a year ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

(Blocks 1 bug)

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

60.51 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
28.35 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
With the upcoming testing of windows mobile and other device without a local webserver, we are looking to add the minimal amount of files on a device other than a basic installation.

A good method for us to run mochitest-chrome, mochitest-browserchrome and mochitest-a11y, is to package those tests up in a .jar file (mochikit.jar) and use it vs the file structure that we currently use.

I have tested this and this works with very few modifications (just to some harness*.xul files in the mochitest root) and a small change to runtests.py when it generates the mochikit.manifest file.

This bug will create a .jar file during 'make package-tests' which will live alongside the existing files in the mochitest directory.
Assignee

Updated

9 years ago
Assignee: nobody → jmaher
Assignee

Comment 1

9 years ago
just a WIP so I don't lose my work.  Need to resolve packaging and clean these changes up in a smart way so they work in both scenarios.
Assignee

Comment 2

9 years ago
just so I don't lose my work, these are the changes I have for a makefile to generate the .jar file.
Assignee

Comment 3

9 years ago
almost complete patch.  This needs a bit more testing and to resolve the best way to turn this on from the runtests.py cli.  ideas?
Attachment #424838 - Attachment is obsolete: true
Attachment #424848 - Attachment is obsolete: true
Attachment #433615 - Flags: feedback?(ted.mielczarek)
Assignee

Comment 4

9 years ago
updated and tested on my winmo phone.  In general this seems to work, although fennec.exe (xul.dll) was crashing when I used the log-file stuff.
Attachment #433615 - Attachment is obsolete: true
Attachment #434022 - Flags: review?(ted.mielczarek)
Attachment #434022 - Flags: feedback?(ctalbert)
Attachment #433615 - Flags: feedback?(ted.mielczarek)

Comment 5

9 years ago
Comment on attachment 434022 [details] [diff] [review]
package up chrome style tests into a .jar file (1)

>diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
>--- a/testing/mochitest/Makefile.in
>+++ b/testing/mochitest/Makefile.in
>@@ -131,17 +131,39 @@ else
>+stage-package: stage-chromejar
> 	$(NSINSTALL) -D $(PKG_STAGE)/mochitest && $(NSINSTALL) -D $(PKG_STAGE)/bin/plugins
> 	@(cd $(DEPTH)/_tests/testing/mochitest/ && tar $(TAR_CREATE_FLAGS) - *) | (cd $(PKG_STAGE)/mochitest && tar -xf -)
> 	@(cd $(DIST_BIN) && tar $(TAR_CREATE_FLAGS) - $(TEST_HARNESS_BINS)) | (cd $(PKG_STAGE)/bin && tar -xf -)
> 	@(cd $(DIST_BIN)/components && tar $(TAR_CREATE_FLAGS) - $(TEST_HARNESS_COMPONENTS)) | (cd $(PKG_STAGE)/bin/components && tar -xf -)
> 	@(cd $(topsrcdir)/build/pgo/certs && tar $(TAR_CREATE_FLAGS) - *) | (cd $(PKG_STAGE)/certs && tar -xf -)
> ifdef MOZ_PLUGINS
> 	@(cd $(DIST_BIN)/plugins && tar $(TAR_CREATE_FLAGS) - $(TEST_HARNESS_PLUGINS)) | (cd $(PKG_STAGE)/bin/plugins && tar -xf -)
> endif
So, if I understand this, we'll be packaging chrome, browser, and a11y twice: once in <package>/mochitest and once in jars.  That's probably not a big issue, but it is a little odd.  I don't know a better way to go about it because you can't specify two makefile targets and we don't want to do a separate package step for tests that might be remote.

Is there any reason to not run them from jars going forward?  Maybe we can make that the standard way of running them?

>diff --git a/testing/mochitest/browser-harness.xul b/testing/mochitest/browser-harness.xul
>--- a/testing/mochitest/browser-harness.xul
>+++ b/testing/mochitest/browser-harness.xul
>@@ -168,23 +168,92 @@
>       /** Find our chrome dir **/
>       var ios = Cc["@mozilla.org/network/io-service;1"].
>                   getService(Ci.nsIIOService);
>       var chromeURI = ios.newURI("chrome://mochikit/content/",
>                                  null, null);
>       var resolvedURI = Cc["@mozilla.org/chrome/chrome-registry;1"].
>                         getService(Ci.nsIChromeRegistry).
>                         convertChromeURL(chromeURI);
>+
>+      if (!resolvedURI.spec.match(/jar:file\/\/\//)) {
>+        return resolvedURI.spec;
>+      }
You really need a comment here.  I just now realized what this code is doing after mxr'ing getChromeDir and staring at its code for a bit (this diff doesn't have function names for some reason).

>+      var testsDir = getChromeDir();
>+      if (typeof(testsDir) == "string") {
>+        var links = getJarListing(testsDir, gConfig.testPath, "browser");
>+        if (links == null) {
>+          return [];
>+        }
>+      } else {
>+        testsDir.appendRelativePath("browser");
This is a little fragile.  Why do you just check if (typeof(testsDir) == string?  Why not ensure that it is actually a spec containing a JAR.  This way if you should get back "undefined" should something go wrong, you won't silently fail.  Also people modifying this code in the future will have a clearer understanding of what is happening here.
Attachment #434022 - Flags: feedback?(ctalbert) → feedback+
(In reply to comment #5)
> Is there any reason to not run them from jars going forward?  Maybe we can make
> that the standard way of running them?

We shouldn't do that in the "running tests from the objdir" situation, because that means you have to rebuild to re-run tests. On platforms with symlinks, you can currently just change the test and re-run it.

I would be fine with having tests-in-jars be the only thing we ship in the test package, however.

Comment 7

9 years ago
(In reply to comment #6)
> (In reply to comment #5)
> > Is there any reason to not run them from jars going forward?  Maybe we can make
> > that the standard way of running them?
> 
> We shouldn't do that in the "running tests from the objdir" situation, because
> that means you have to rebuild to re-run tests. On platforms with symlinks, you
> can currently just change the test and re-run it.
> 
> I would be fine with having tests-in-jars be the only thing we ship in the test
> package, however.

Oops, yes I completely agree with you.  In comment 5 I was only thinking about the run from package scenario.  I had already assumed that running from objdir shouldn't change.  And you know what they say about assuming ;)
Assignee

Comment 8

9 years ago
I would like to address the single packaging solution in a followup bug once this lands for a couple reasons:

1) lets get this stable and not mess with something that works
2) there might be other things we determine should go this route
3) this way we don't hose the world on accident!

ctalbert, thanks for the feedback, I appreciate it.  

On a related note, I have found that using the .jar method allows for --test-path for a dir or file on both chrome and browser chrome tests.  I am tinkering with this for non .jar files.
Comment on attachment 434022 [details] [diff] [review]
package up chrome style tests into a .jar file (1)

>diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
>--- a/testing/mochitest/Makefile.in
>+++ b/testing/mochitest/Makefile.in
>@@ -131,17 +131,39 @@ else
> TEST_HARNESS_PLUGINS := \
>   $(DLL_PREFIX)nptest$(DLL_SUFFIX)
> endif
> 
> # Rules for staging the necessary harness bits for a test package
> PKG_STAGE = $(DIST)/test-package-stage
> DIST_BIN = $(DIST)/bin
> 
>-stage-package:
>+PKG_CHROMEJAR = $(DEPTH)/_tests/testing/mochitest/content/
>+
>+stage-chromejar:
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)/browser
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)/tests/SimpleTest
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)/chrome
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)/a11y
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)/MochiKit
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)/static

Why do you need to create all these subdirs? Won't the cp -R below take care of that for you?

>+	cp -R $(DEPTH)/_tests/testing/mochitest/browser $(PKG_CHROMEJAR)
>+	cp -R $(DEPTH)/_tests/testing/mochitest/chrome $(PKG_CHROMEJAR)
>+	cp -R $(DEPTH)/_tests/testing/mochitest/a11y $(PKG_CHROMEJAR)
>+	cp -R $(DEPTH)/_tests/testing/mochitest/MochiKit $(PKG_CHROMEJAR)
>+	cp -R $(DEPTH)/_tests/testing/mochitest/tests/SimpleTest $(PKG_CHROMEJAR)/tests
>+	cp -R $(DEPTH)/_tests/testing/mochitest/static $(PKG_CHROMEJAR)
>+	cp $(DEPTH)/_tests/testing/mochitest/browser-* $(PKG_CHROMEJAR)
>+	cp $(DEPTH)/_tests/testing/mochitest/*harness* $(PKG_CHROMEJAR)
>+	cp $(DEPTH)/_tests/testing/mochitest/server.js $(PKG_CHROMEJAR)
>+	@(cd $(DEPTH)/_tests/testing/mochitest && zip -r mochikit.jar content/)

It's a bit weird that this modifies stuff in the objdir. The stage-package stuff below uses a special staging directory to avoid touching the tree. This means that you could wind up with different behavior before and after running "make package-tests".

I still really think you should just work this into the stage-package thing and make it the default for packaged tests. If it's not working well enough to be the default then it's not working very well anyway, is it?

>diff --git a/testing/mochitest/browser-harness.xul b/testing/mochitest/browser-harness.xul
>--- a/testing/mochitest/browser-harness.xul
>+++ b/testing/mochitest/browser-harness.xul
>@@ -168,23 +168,92 @@
>       /** Find our chrome dir **/
>       var ios = Cc["@mozilla.org/network/io-service;1"].
>                   getService(Ci.nsIIOService);
>       var chromeURI = ios.newURI("chrome://mochikit/content/",
>                                  null, null);
>       var resolvedURI = Cc["@mozilla.org/chrome/chrome-registry;1"].
>                         getService(Ci.nsIChromeRegistry).
>                         convertChromeURL(chromeURI);
>+
>+      if (!resolvedURI.spec.match(/jar:file\/\/\//)) {
>+        return resolvedURI.spec;
>+      }

I don't like that this function can return either a URI string or a nsILocalFile. That seems odd. Why not just always return the URI and make the caller deal with it? Then they can check the uri.scheme to see what they have. You'd probably want to change the name of this function as well, then.

>+    function getJarListing(uri, testPath, dir)
>+    {
>+      var zReader = Cc["@mozilla.org/libjar/zip-reader;1"].
>+                      createInstance(Components.interfaces.nsIZipReader);
>+
>+      var parts = uri.split('!');
>+      var names = parts[0].split('///');
>+      var name = "/" + names[names.length - 1];

This is ugly. If you keep the nsIJARURI around, you can just ask it for its .JARFile:
http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsIJARURI.idl#57

>+
>+      var fileName = Cc["@mozilla.org/file/local;1"].
>+                       createInstance(Ci.nsILocalFile);
>+
>+      try {
>+        fileName.initWithPath(name);

And then you should use fileHandler.getFileFromURLSpec to get it to a nsIFile.

>+      zReader.open(fileName);
>+      var requestPath = "chrome://mochikit/";
>+      var base = "content/" + dir + "/*";
>+
>+      if (testPath) {

I think it would be clearer if the caller handled testPath, and just passed a JAR file and a path into here.

>+      dump("checking path: " + base + "\n\n");
>+      var files = zReader.findEntries(base + "*");

This reads strangely with all the early returns in the test path checking above.

>+      var links = {};
>+      var fileArray = new Array();

You can just say fileArray = [];

>+      while(files.hasMore()) {
>+        var entryName = files.getNext();
>+        fileArray.push(requestPath + entryName);
>+      }
>+      fileArray.sort();
>+      for (var i=0; i < fileArray.length; i++) {
>+        var file = fileArray[i];
>+        links[file] = true;

This is kind of weird. Doesn't the "list" function from server.js actually build a nested list of links?

>+      var testsDir = getChromeDir();
>+      if (typeof(testsDir) == "string") {

As I said before, this is ugly.

>diff --git a/testing/mochitest/harness-overlay.xul b/testing/mochitest/harness-overlay.xul
>--- a/testing/mochitest/harness-overlay.xul
>+++ b/testing/mochitest/harness-overlay.xul
>@@ -13,73 +13,153 @@
>           src="chrome://mochikit/content/tests/SimpleTest/TestRunner.js"/>
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/MozillaFileLogger.js"/>
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/quit.js" />
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/setup.js" />
>   <script type="application/javascript;version=1.7"><![CDATA[
>-    function loadTests()
>+
>+    function getJarListing(uri, params, dir)

Please find a way to share this code, copy/pasting it sucks.

>diff --git a/testing/mochitest/runtestsremote.py b/testing/mochitest/runtestsremote.py
>--- a/testing/mochitest/runtestsremote.py
>+++ b/testing/mochitest/runtestsremote.py
>@@ -119,16 +119,22 @@ class RemoteOptions(MochitestOptions):
>                     help = "ip address where the remote web server is hosted at")
>         defaults["httpPort"] = automation.DEFAULT_HTTP_PORT
> 
>         self.add_option("--ssl-port", action = "store",
>                     type = "string", dest = "sslPort",
>                     help = "ip address where the remote web server is hosted at")
>         defaults["sslPort"] = automation.DEFAULT_SSL_PORT
> 
>+
>+        self.add_option("--run-chrome-jar", action = "store_true",
>+                    dest = "runChromeJar",
>+                    help = "Run chrome style tests as a .jar file")
>+        defaults["runChromeJar"] = True

Seems like you should just detect if the jar exists and use it, if so, instead of having an option.
Attachment #434022 - Flags: review?(ted.mielczarek) → review-
Posted patch mochikit.jar (2) (obsolete) — Splinter Review
this is cleaning up well.  I am running through try server as I type this, but wanted to get something uploaded in case people are interested in this patch or status of it.

Any feedback is appreciated, and I will update patch if necessary after getting green on try server.
Attachment #434022 - Attachment is obsolete: true
Attachment #452344 - Flags: feedback?(ctalbert)
ok, with my latest patch (defaults to mochikit.jar on tinderbox), I have 45 chrome failures and 2 browser-chrome failures (0 a11y failures!)

It looks like a few of these failures (maybe half) are related to:
 - unable to find a script
 - expecting a file URI or profile extension

So the next challenge is for me to put this into the profile (like reftest) and run that way.  This is our requirement for Android.

Comment 12

9 years ago
Comment on attachment 452344 [details] [diff] [review]
mochikit.jar (2)

You've got a whole lot of histrionics in this code dealing with getting a URL to the jar file.  Why don't you write a real extension with a real install.rdf (it can be checked in to the tree, see reftest: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/install.rdf, and install normally with mochikit.jar inside an extension.  Then the path to your extension files would be: chrome://mochikit_ext/content/SimpleTest/simpletest.js

assuming you have a manifest like:
content mochitest_ext jar:mochitest.jar!content/

I think if we're making this into an extension we should go all the way.  The strange half-way case is going to only be good for finding corner cases in our code and shooting ourselves in the foot, I'm afraid.
Attachment #452344 - Flags: feedback?(ctalbert) → feedback+
I confirm that the sqlite database used for template tests should be in the profile or somewhere into the file system, not in a chrome package. So only sqlite databases accessible by a file:// or profile: URI are allowed.
Assignee

Updated

9 years ago
Depends on: 574189
Posted patch mochikit.jar WIP (3.0) (obsolete) — Splinter Review
this patch actually work well on tinderbox making chrome/browser-chrome/a11y runs use mochikit.jar.  The problem is packaging up the harness + tests.  This patch does that and it works great as a single .jar file, but for android, we need to install the .jar in the profile which requires a .rdf file and directory structure.  You can see some work I did in this patch to do that, but jar.mn takes a static list of files instead of a directory structure.
Attachment #452344 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Depends on: 581828
Assignee

Updated

9 years ago
Depends on: 591325
Assignee

Updated

9 years ago
Depends on: 592166
Assignee

Updated

9 years ago
Depends on: 592859
Assignee

Updated

9 years ago
Duplicate of this bug: 536407
Posted patch mochikit.jar (3.1) (obsolete) — Splinter Review
updated patch to work with tests.  This works like this:
 - during make we create a mochijar directory which contains the mochikit extension (chrome://mochikit/content).  This will be used in all scenarios where we use chrome/browser/a11y tests.  mochijar only contains the core harness bits and utility/libraries that we depend on
 - during make package-tests create a tests.jar file.  If this exists, we put this in the profile and generate a tests.manifest.  This gets dynamically registered (from inside the profile) and we can access the tests via chrome://mochitests/content.  If there is no tests.jar, we add chrome://mochitests/content to the mochijar extension chrome.manifest file.

With this patch there should be no hardcoded chrome://mochikit for any test data (other than packed.js and SimpleTest utilities)
Attachment #460160 - Attachment is obsolete: true
Attachment #474359 - Flags: review?(ted.mielczarek)
Attachment #474359 - Flags: feedback?(ctalbert)
this patch is required for mochikit.jar to run.  If additional tests are added between now and landing we might have to modify them as well.
Attachment #474360 - Flags: review?(ctalbert)
I am still waiting on Windows results, but the last set of results were identical to OSX, so I will count on that being the same.

I also have been seeing a postflight error during build on OSX opt:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1284178856.1284189055.26318.gz

I am not sure if this is related to my patch or not.  I will continue to look into it.

Comment 19

9 years ago
Previous versions of this work (changing the chrome URLs for chrome and browser-chrome tests) had the side-effect of disabling huge swaths of tests. Docshell ended up disabled, plus a couple of others.  We need to ensure that the set of tests run prior to the patch landing and the set run afterward must be the same.

We need to be careful about this before landing this patch, especially since disabling tests is a completely invisible failure in our harnesses.
I have looked at all tests (listing from server.js and my .jar listing toolset) from august 23, 2010 until today (sept 20, 2010) and compared the lists.

There are many differences, but all files appear to be files that have been added/removed in the last ~4 weeks.

This patch is not unintentionally disabling tests or directories.

Comment 21

9 years ago
Comment on attachment 474359 [details] [diff] [review]
mochikit.jar (3.1)

Awesome, thanks for checking on that.  I think this part looks good.  Will review the second piece this afternoon.
Attachment #474359 - Flags: feedback?(ctalbert) → feedback+
updated for new and updated tests since my last try server run.  Passed on a try server run from 9/22/2010 am :)
Attachment #474360 - Attachment is obsolete: true
Attachment #477540 - Flags: review?(ctalbert)
Attachment #474360 - Flags: review?(ctalbert)
Comment on attachment 474359 [details] [diff] [review]
mochikit.jar (3.1)

>diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
>--- a/testing/mochitest/Makefile.in
>+++ b/testing/mochitest/Makefile.in
>@@ -46,16 +46,28 @@ include $(DEPTH)/config/autoconf.mk
> DIRS =	MochiKit \
> 	static \
> 	dynamic \
> 	tests \
> 	chrome \
> 	ssltunnel \
> 	$(NULL)
> 
>+
>+NO_JS_MANIFEST = 1
>+MOZ_CHROME_FILE_FORMAT = jar
>+DIST_FILES = install.rdf
>+
>+# Used in install.rdf
>+USE_EXTENSION_MANIFEST=1
>+
>+XPI_NAME=mochijar

nit: spaces around the = in variable assignments.

>+libs:: mochijar

You don't need a separate mochijar target, you can just do
libs::
    <your rule here>

>+
>+
> include $(topsrcdir)/config/rules.mk
> # We're installing to _tests/testing/mochitest, so this is the depth
> # necessary for relative objdir paths.
> TARGET_DEPTH = ../../..
> include $(topsrcdir)/build/automation-build.mk
> 
> # files that get copied into $objdir/_tests/
> _SERV_FILES = 	\
>@@ -170,17 +182,37 @@ else
> TEST_HARNESS_PLUGINS := \
>   $(DLL_PREFIX)nptest$(DLL_SUFFIX)
> endif
> 
> # Rules for staging the necessary harness bits for a test package
> PKG_STAGE = $(DIST)/test-package-stage
> DIST_BIN = $(DIST)/bin
> 
>-stage-package:
>+PKG_CHROMEJAR = $(PKG_STAGE)/mochitest/content/
>+
>+stage-chromejar:
>+	$(NSINSTALL) -D $(PKG_CHROMEJAR)
>+	cp -RL $(DEPTH)/_tests/testing/mochitest/browser $(PKG_CHROMEJAR)
>+	cp -RL $(DEPTH)/_tests/testing/mochitest/chrome $(PKG_CHROMEJAR)
>+ifneq (Darwin,$(OS_TARGET))
>+	cp -RL $(DEPTH)/_tests/testing/mochitest/a11y $(PKG_CHROMEJAR)

I think you want ifdef ACCESSIBILITY here.

>+stage-package: stage-chromejar mochijar

Why are you running the mochijar target again during stage-package? If it's getting run during libs:: that should be sufficient.

>diff --git a/testing/mochitest/browser-harness.xul b/testing/mochitest/browser-harness.xul
>--- a/testing/mochitest/browser-harness.xul
>+++ b/testing/mochitest/browser-harness.xul
>@@ -203,25 +203,45 @@
>           return "<p class=\"" + class + "\">" + t.result + " | " + path +
>                  " | " + _entityEncode(t.msg) + "</p>";
>         }).join("\n");
>       }
>     };
> 
>     // Returns an array of browserTest objects for all the selected tests
>     function listTests() {
>+      var baseURL = 'chrome://mochikit';

I'm not wild about all the baseURL + "/content" you wind up doing. Could you at least assign baseURL + "/content" to a new variable and use that?

>+      var jar = getJar(baseURL + '/content');
>+      if (jar != null) {
>+        var testsURI = Components.classes["@mozilla.org/file/directory_service;1"]
>+                            .getService(Components.interfaces.nsIProperties)
>+                            .get("ProfD", Components.interfaces.nsILocalFile);
>+        testsURI.append("tests.manifest");
>+        var ioSvc = Components.classes["@mozilla.org/network/io-service;1"].
>+                    getService(Components.interfaces.nsIIOService);
>+        var manifestFile = ioSvc.newURI("file://" + testsURI.path, null, null)
>+                        .QueryInterface(Components.interfaces.nsIFileURL).file;

You just want to use newFileURI here:
https://developer.mozilla.org/en/nsIIOService#newFileURI.28.29

Alternately, you can just Components.utils.import("resource://gre/modules/NetUtil.jsm"); and use its newURI method (which can take an nsIFile):
https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm


>diff --git a/testing/mochitest/harness-overlay.xul b/testing/mochitest/harness-overlay.xul
>--- a/testing/mochitest/harness-overlay.xul
>+++ b/testing/mochitest/harness-overlay.xul
>@@ -12,86 +12,74 @@
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/TestRunner.js"/>
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/MozillaFileLogger.js"/>
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/quit.js" />
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/setup.js" />
>+  <script type="application/javascript;version=1.7"
>+          src="chrome://mochikit/content/chrome-harness.js" />

I don't think these version statements are needed anymore. I'm pretty sure we default the JS version to LATEST.

>diff --git a/testing/mochitest/jar.mn b/testing/mochitest/jar.mn
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/jar.mn
>@@ -0,0 +1,39 @@
>+mochikit.jar:
>+% content mochikit %content/
>+  content/browser-harness.xul (browser-harness.xul)
>+  content/browser-test.js (browser-test.js)
>+  content/browser-test-overlay.xul (browser-test-overlay.xul)
>+  content/chrome-harness.js (chrome-harness.js)
>+  content/harness-a11y.xul (harness-a11y.xul)
>+  content/harness-overlay.xul (harness-overlay.xul)
>+  content/harness.xul (harness.xul)
>+  content/ipc.js (ipc.js)
>+  content/ipc-overlay.xul (ipc-overlay.xul)
>+  content/mozprefs.js (mozprefs.js)
>+  content/redirect-a11y.html (redirect-a11y.html)
>+  content/redirect.html (redirect.html)
>+  content/redirect.js (redirect.js)
>+  content/server.js (server.js)
>+  content/dynamic/getMyDirectory.sjs (dynamic/getMyDirectory.sjs)
>+  content/static/bug100533_iframe.html (static/bug100533_iframe.html)
>+  content/static/bug100533_load.html (static/bug100533_load.html)
>+  content/static/bug277724_iframe1.html (static/bug277724_iframe1.html)
>+  content/static/bug277724_iframe2.xhtml (static/bug277724_iframe2.xhtml)
>+  content/static/bug340800_iframe.txt (static/bug340800_iframe.txt)
>+  content/static/bug344830_testembed.svg (static/bug344830_testembed.svg)

Are these just old tests that still live in the mochitest dir? We should finish getting random tests the heck out of the Mochitest dir.

>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in
>@@ -567,16 +567,18 @@ class Mochitest(object):
> 
>     self.leak_report_file = os.path.join(options.profilePath, "runtests_leaks.log")
> 
>     browserEnv = self.buildBrowserEnv(options)
>     if (browserEnv == None):
>       return 1
> 
>     manifest = self.buildProfile(options)
>+    if (manifest == None):
>+      return 1

No parens, and "if manifest is None".

>-    self.cleanup(manifest, options)
>+    if (manifest != None):
>+      self.cleanup(manifest, options)

Same thing, but "if manifest is not None:"

>+    #TODO: test in tests.jar scenario and no 'mochikit' scenario

I don't understand this TODO comment. Also, if it's an actual TODO, file a bug and include a bug number.

>+    if self.installTestsJar(options):
>+      manifestFile.write("content mochitests jar:tests.jar!/content/\n");
>+    else:
>+      manifestFile.write("content mochitests " + chrometestDir + " contentaccessible=yes\n")

I prefer string substition: "content mochitests %s contentaccessible=yes\n" % chrometestDir 

>+  def installChromeJar(self, jarDirName, browser_chrome, options):

Add a docstring comment, please.

>+    jarDir = os.path.join(options.profilePath, 'extensions', 'mochikit@mozilla.org')
>+    shutil.copytree(os.path.join(self.SCRIPT_DIRECTORY, jarDirName), jarDir)
>+    mfile = open(os.path.join(jarDir, "chrome.manifest"), 'a')
>+    mfile.write(browser_chrome)
>+    mfile.close()

We require Python 2.5 now, so you can do:
with open(os.path.join(jarDir, "chrome.manifest"), 'a') as mfile:
  mfile.write(browser_chrome)

(You'll need to add "from __future__ import with_statement" to the imports section up top to use this with 2.5.)
>+    return jarDir
>+
>+  def installTestsJar(self, options):

docstring comment!

r=me with those things fixed.
Attachment #474359 - Flags: review?(ted.mielczarek) → review+
Assignee

Updated

9 years ago
Depends on: 599847
Posted patch mochikit.jar (3.14) (obsolete) — Splinter Review
updated to address review comments.
Attachment #474359 - Attachment is obsolete: true
Attachment #479029 - Flags: review+
ok, I have a solution for the universal_binary workaround that is green on tryserver.  Here is a diff of it:
http://pastebin.mozilla.org/800413

Comment 26

9 years ago
Comment on attachment 477540 [details] [diff] [review]
final set of tests that need to be modified for running from chrome://mochitests (1.1)

>diff --git a/toolkit/crashreporter/test/browser/browser_aboutCrashes.js b/toolkit/crashreporter/test/browser/browser_aboutCrashes.js
>--- a/toolkit/crashreporter/test/browser/browser_aboutCrashes.js
>+++ b/toolkit/crashreporter/test/browser/browser_aboutCrashes.js
>@@ -1,14 +1,14 @@
> // load our utility script
> var scriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>                              .getService(Components.interfaces.mozIJSSubScriptLoader);
> 
> var rootDir = getRootDirectory(gTestPath);
>-scriptLoader.loadSubScript(rootDir + "/aboutcrashes_utils.js", this);
>+scriptLoader.loadSubScript(rootDir + "aboutcrashes_utils.js", this);
Note that when we give getRootDirectory a path, it must return with a trailing '/'.

>diff --git a/xpinstall/tests/browser_auth.js b/xpinstall/tests/browser_auth.js
>--- a/xpinstall/tests/browser_auth.js
>+++ b/xpinstall/tests/browser_auth.js
>@@ -1,12 +1,14 @@
> // Load in the test harness
> var scriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>                              .getService(Components.interfaces.mozIJSSubScriptLoader);
>-scriptLoader.loadSubScript("chrome://mochikit/content/browser/xpinstall/tests/harness.js", this);
>+
>+var rootDir = getRootDirectory(window.location.href);
>+scriptLoader.loadSubScript(rootDir + "/harness.js", this);
However, when we give getRootDirectory a URL, it doesn't ensure there is a trailing '/', forcing us to append that manually.

I would prefer that getRootDirectory always append that trailing '/' so that people don't have to guess at its behavior.  And I can't think of any reason where we would *not* want a trailing '/', so let's do that across the board.

r+ with that change (and the resulting changes to remove the manually appended '/' in the rest of these xpinstall tests

Otherwise, this patch looks good.
Attachment #477540 - Flags: review?(ctalbert) → review+
Posted patch mochikit.jar (3.15) (obsolete) — Splinter Review
ted, asking for a quick review here.  This is just the original review + comments + universal_binary fix for mac osx opt builds.

I am mostly interested in your feedback on the universal_binary fix in these files:
build/macos/universal/flight.mk
testing/testsuite-targets.mk
testing/mochitest/Makefile.in
Attachment #479029 - Attachment is obsolete: true
Attachment #479738 - Flags: review?(ted.mielczarek)
addressed the trailing slash issue and ran through try server for sanity sakes.
Attachment #477540 - Attachment is obsolete: true
Attachment #479739 - Flags: review+
Comment on attachment 479738 [details] [diff] [review]
mochikit.jar (3.15)

>diff --git a/build/macosx/universal/flight.mk b/build/macosx/universal/flight.mk
>--- a/build/macosx/universal/flight.mk
>+++ b/build/macosx/universal/flight.mk
>@@ -116,18 +116,18 @@ postflight_all:
>           --unify-with-sort "\.manifest$$" \
>           --unify-with-sort "components\.list$$" \
> 	  $(DIST_ARCH_1)/$(MOZ_PKG_APPNAME)/$(APPNAME) \
> 	  $(DIST_ARCH_2)/$(MOZ_PKG_APPNAME)/$(APPNAME) \
> 	  $(DIST_UNI)/$(MOZ_PKG_APPNAME)/$(APPNAME)
> # A universal .dmg can now be produced by making in either architecture's
> # INSTALLER_DIR.
> # Now, repeat the process for the test package.
>-	$(MAKE) -C $(OBJDIR_ARCH_1) UNIVERSAL_BINARY= package-tests
>-	$(MAKE) -C $(OBJDIR_ARCH_2) UNIVERSAL_BINARY= package-tests
>+	$(MAKE) -C $(OBJDIR_ARCH_1) UNIVERSAL_BINARY= package-tests CHROME_JAR=0
>+	$(MAKE) -C $(OBJDIR_ARCH_2) UNIVERSAL_BINARY= package-tests CHROME_JAR=0

You could use "CHROME_JAR=" here, which should make it undefined, so you could test it with "ifdef CHROME_JAR". That's more in line with the UNIVERSAL_BINARY handling.

Also, could you put both variable definitions on the same side of the target name (package-tests)?

>diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
>--- a/testing/mochitest/Makefile.in
>+++ b/testing/mochitest/Makefile.in
>+PKG_CHROMEJAR = $(PKG_STAGE)/mochitest/content/
>+
>+ifeq ($(CHROME_JAR),1)

If you follow my suggestion above, this could just be ifdef CHROME_JAR.

>+stage-chromejar:

>diff --git a/testing/testsuite-targets.mk b/testing/testsuite-targets.mk
>--- a/testing/testsuite-targets.mk
>+++ b/testing/testsuite-targets.mk
>@@ -137,18 +137,22 @@ ifndef UNIVERSAL_BINARY
> PKG_STAGE = $(DIST)/test-package-stage
> package-tests: stage-mochitest stage-reftest stage-xpcshell stage-jstests stage-mozmill stage-jetpack
> else
> # This staging area has been built for us by universal/flight.mk
> PKG_STAGE = $(DIST)/universal/test-package-stage
> endif
> 
> package-tests:
>+ifndef UNIVERSAL_BINARY
> 	$(NSINSTALL) -D $(DIST)/$(PKG_PATH)
> 	@rm -f "$(DIST)/$(PKG_PATH)$(TEST_PACKAGE)"

Keep the rm outside of the ifdef, just in case the file is already there you want to delete it.

>+else
>+	$(MAKE) -C $(DEPTH)/testing/mochitest stage-chromejar PKG_STAGE=$(DIST)/universal
>+endif

This could probably use a comment describing why we're doing things this way.

r=me with those nits fixed.
Attachment #479738 - Flags: review?(ted.mielczarek) → review+
minor update to address :ted's review feedback.
Attachment #479738 - Attachment is obsolete: true
Attachment #479878 - Flags: review+

Comment 31

9 years ago
http://hg.mozilla.org/mozilla-central/rev/0f998c9f8728
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 601113
Attachment #479739 - Attachment description: final set of tests that need to be modified for running from chrome://mochitests (1.2) → final set of tests that need to be modified for running from chrome://mochitests (1.2) [Checked in: Comment 31]
Comment on attachment 479878 [details] [diff] [review]
mochikit.jar (3.16)
[Checked in: Comment 32]

http://hg.mozilla.org/mozilla-central/rev/7e4f8cb08cba
Attachment #479878 - Attachment description: mochikit.jar (3.16) → mochikit.jar (3.16) [Checked in: Comment 32]
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b7
Depends on: 601069
Depends on: 600066
Depends on: 603233

Comment 33

9 years ago
Hi, this changes seriously broke the browser-chrome tests or I do not know how to run them now
Currently if I run runtests.py with --browser-chrome option, all the chrome tests running but not the browser-chrome tests, if I package browser chrome tests into tests.jar and replace the original one, most of the previously passing tests failed.
Depends on: 608546
No longer depends on: 603233
Component: New Frameworks → General
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.