Closed Bug 631052 Opened 11 years ago Closed 11 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file, 3 obsolete files)

As testing restarts is difficult in all our other test suites mozmill is the best way to test bug 630703
Blocks: 630703
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #509270 - Flags: review?(hskupin)
Attached patch patch rev 2 (obsolete) — Splinter Review
Updated for the naming changes from the implementation bug.
Attachment #509270 - Attachment is obsolete: true
Attachment #509292 - Flags: review?(hskupin)
Attachment #509270 - Flags: review?(hskupin)
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.
Attachment #509292 - Flags: review?(hskupin) → review-
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Summary: Implement test for bug 630703 → [Mac] Implement Mozmill test for restarting the application in 32/64 bit mode
Attached patch patch rev 3 (obsolete) — Splinter Review
(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.
Attachment #509292 - Attachment is obsolete: true
Attachment #509596 - Flags: review?(hskupin)
(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.
Attachment #509596 - Flags: review?(hskupin) → review-
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.
Attached patch patch rev 4Splinter Review
Attachment #509596 - Attachment is obsolete: true
Attachment #510762 - Flags: review?(hskupin)
Attachment #510762 - Flags: review?(hskupin) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(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.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.