631052 - [Mac] Implement Mozmill test for restarting the application in 32/64 bit mode

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Townsend [:mossop]]

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

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated for the naming changes from the implementation bug.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 3

(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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 8

[Dave Townsend [:mossop]]

(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.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 11

[Dave Townsend [:mossop]]

(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.