As testing restarts is difficult in all our other test suites mozmill is the best way to test bug 630703

The first test checks if the test is running on OSX and has started in 64-bit mode. if either isn't true then it skips the tests entirely. The tests here verify that on first run it is in 64-bit mode, a restart leaves it in 64-bit mode, a restart to 32-bit mode works, a restart leaves it in 32-bit mode and then a restart to 64-bit mode works.

Updated for the naming changes from the implementation bug.

Comment on attachment 509292 [details] [diff] [review] patch rev 2 >+ * Portions created by the Initial Developer are Copyright (C) 2009 Please update the date to 2011 in all of your files. >+var setupModule = function(module) { You have probably grabbed an old test as template. For new tests we do not use anonymous functions anymore. >+var testArchitecture = function() >+{ The test functions should have a unique name across the files. The best is to use a naming which directly associates what is being tested, i.e: testArchitectureRestartNormal testArchitectureRestartIn32 testArchitectureRestartIn64 [...] Also please put the bracket in the same line as the function definition. >+ var runtime = Cc["@mozilla.org/xre/runtime;1"]. >+ getService(Ci.nsIXULRuntime); The service has already been retrieved at the top. Why not using the global variable? >+ controller.assert(function () { >+ return runtime.XPCOMABI === "x86_64-gcc3"; >+ }, "ABI ahould be 64-bit"); We currently don't have a got/expected output. So you will have to add it on your own to the message in the following format: " - got 'xyz', expected 'xyz'". It will change in the next round of module refactoring. >+var testRestart = function() >+{ >+ controller.startUserShutdown(4000, true); >+ var appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]. >+ getService(Ci.nsIAppStartup); >+ appStartup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); >+} Move that code to the teardownModule function. It's not really a test. Otherwise it looks fine, but I can't fully test it as long as the patch has not been checked-in. We also have to wait with the check-in until bug 630703 has been fixed.

(In reply to comment #3) > >+var setupModule = function(module) { > > You have probably grabbed an old test as template. For new tests we do not use > anonymous functions anymore. I took the base from the tutorial, it probably needs to be updated: https://developer.mozilla.org/en/Mozmill/First_Steps/Tutorial%3a_Introduction_to_Mozmill > >+var testRestart = function() > >+{ > >+ controller.startUserShutdown(4000, true); > >+ var appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]. > >+ getService(Ci.nsIAppStartup); > >+ appStartup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); > >+} > > Move that code to the teardownModule function. It's not really a test. I had to put it in teardownTest instead, in teardownModule it would fail with the error "Test Failure: {"function": "Runner.wrapper", "message": "Shutdown expected but none detected before end of test"}" Also added a test to see that its running in a universal build since non-UB can't change architecture either.

(In reply to comment #4) > I took the base from the tutorial, it probably needs to be updated: > https://developer.mozilla.org/en/Mozmill/First_Steps/Tutorial%3a_Introduction_to_Mozmill This page needs a complete update. It's misleading and hasn't been updated for ages. > I had to put it in teardownTest instead, in teardownModule it would fail with > the error "Test Failure: {"function": "Runner.wrapper", "message": "Shutdown > expected but none detected before end of test"}" As figured out yesterday on IRC, the startUserShutdown call has to be in the same test function. So I think that's fine. > Also added a test to see that its running in a universal build since non-UB > can't change architecture either. Good catch.

Comment on attachment 509596 [details] [diff] [review] patch rev 3 >+/* Nit: Please use '/**' for function comments. Not sure how this would affect jsdoc generated content, but we want to do that in the near future. >+var testArchitecture64bit = function() { Seems like you missed to change it to a named function. >+ controller.assert(function () { >+ return runtime.XPCOMABI === "x86_64-gcc3"; >+ }, "ABI should be 'x86_64-gcc3' but got '" + runtime.XPCOMABI + "'"); We want to keep the same style across all tests for the assert messages. If you compare against a fixed value, just use: "%message% - got 'xyz' For the final review I will have to wait until the builds are publicly are available.

Dave, will you have time to fix the remaining comments? Would be nice to get this test landed, now that bug 630703 has been fixed.

(In reply to comment #7) > Dave, will you have time to fix the remaining comments? Would be nice to get > this test landed, now that bug 630703 has been fixed. Yep, should have a new patch up today or tomorrow.

Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/81279d5efef1 Dave, do we also wanna have a Litmus test? IMO it would be quite a complicated one and I think the Mozmill test should be enough.

(In reply to comment #10) > Landed as: > http://hg.mozilla.org/qa/mozmill-tests/rev/81279d5efef1 > > Dave, do we also wanna have a Litmus test? IMO it would be quite a complicated > one and I think the Mozmill test should be enough. I think there should be a litmus case covering using the feature in bug 628651 but otherwise I don't think we need anything specific for this API.