886811 - Use controller.restartApplication() or controller.stopApplication() instead of controller.startUserShutdown() for mozmill-2.0

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andrei Eftimie]

controller.startUserShutdown() is partially broken as per bug 886360 and will be fixed for mozmill-2.1 

As a workaround we should use controller.restartAppplication()

Comment 1

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

Lets get this done when I have landed the remaining patch for bug 865690.

Comment 2

[Andrei Eftimie]

Attachment: Patch v1

I've replaced startUserShutdown() with restartApplication()

1) In this context testMenu_quitApplication/test1.js does not make any sense.
It is testing Firefox quit option from the menu, so I've left startUserShutdown() here.

2) OSX restartChangeArchitecture uses custom restarts so I've left them for 1.5 and changed restartApplication() to receive those custom restartFlags in bug 888853

Testrun 1.5 http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e6c4db5
Tesrun 2.0 (with patch from bug 888853 applied) http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e69e99d

Comment 3

[Andrei Eftimie]

Well, it seems this won't do it for the OSX restartChangeArchitecture tests.
For those only I still get "Attempting to send message after session closed" so we might as well leave them with the old userShutDown() method, and scrap bug 888853 :(

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andrei Eftimie from comment #3)
> Well, it seems this won't do it for the OSX restartChangeArchitecture tests.
> For those only I still get "Attempting to send message after session closed"
> so we might as well leave them with the old userShutDown() method, and scrap
> bug 888853 :(

Is the review request still required? It's unclear to me from this comment.

Comment 5

[Andrei Eftimie]

I'm not sure what approach we should take here (in tandem with bug 888853).
I'de wait for Henrik's input on this one since he is working on the restart problems in mozmill atm

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Okay, let's wait for Henrik to return from PTO

Comment 7

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

Comment on attachment 770172 [details] [diff] [review]
Patch v1

Review of attachment 770172 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ -81,5 @@
> -  var restartLink = addonsManager.getElement({type: "listView_restartLink",
> -                                              parent: plainTheme});
> -
> -  controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true);
> -  controller.click(restartLink);

Don't remove those lines! Only comment them out, as what i have mentioned earlier on this bug (at least as far as I can remember).

Comment 8

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

(In reply to Andrei Eftimie from comment #2)
> 1) In this context testMenu_quitApplication/test1.js does not make any sense.
> It is testing Firefox quit option from the menu, so I've left
> startUserShutdown() here.

Why not? Use stopApplication() instead.

> 2) OSX restartChangeArchitecture uses custom restarts so I've left them for
> 1.5 and changed restartApplication() to receive those custom restartFlags in
> bug 888853

This is diminishment now. :) It's already available through bug 865690.

Comment 9

[Andrei Eftimie]

Attachment: Patch v2

I've left the original startUserShutdown() calls for mozmill 1.5 and updated the description of the TODO to this bug.

In regard to the testMenu_quitApplication/test1.js
> Why not? Use stopApplication() instead.

I have changed it to use stopApplication() for mozmill 2, but as I've initially said, the test is meant to test the UI, so by calling an API we kind of defeat its purpose. Anyway, that test was (and stil is) skipped, so it will probably need a later revisiting.

> This is diminishment now. :) It's already available through bug 865690.
That wasn't landed when I uploaded the previous patch :P

We still have a problem here, that's why I am only asking for feedback.
I'll shortly upload a patch for mozmill, since the OSX restartChangeArchitecture tests won't work with only the current changes. I'll add further information besides that patch.

Other than this we should be good.

Comment 10

[Andrei Eftimie]

Attachment: mozmill test patch

Changes needed for OSX restartChangeArchitecture tests.
This patch is required for the previous uploaded patch to work.

We never seem to call Services.startup.quit() with the required flags, so the architecture change never takes place.

I'm not entirely fond of how I solved this problem here.
Basically this would allow restartApplication() to issue a "user-restart" and will most probably require a timeout in that case. And this may be way to similar to startUserShutDown(), but I have not found a way to trigger the startup.quit() call with custom flags that actually works.

Any ideas?

Thanks.

Comment 11

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

(In reply to Andrei Eftimie from comment #9)
> In regard to the testMenu_quitApplication/test1.js
> > Why not? Use stopApplication() instead.
> 
> I have changed it to use stopApplication() for mozmill 2, but as I've
> initially said, the test is meant to test the UI, so by calling an API we
> kind of defeat its purpose. Anyway, that test was (and stil is) skipped, so
> it will probably need a later revisiting.

That's correct and that's why the test is skipped. Lets hope we can re-enable it with Mozmill 2.1 following soon.

> > This is diminishment now. :) It's already available through bug 865690.
> That wasn't landed when I uploaded the previous patch :P
> 
> We still have a problem here, that's why I am only asking for feedback.
> I'll shortly upload a patch for mozmill, since the OSX
> restartChangeArchitecture tests won't work with only the current changes.
> I'll add further information besides that patch.

Hm, interesting. I'm curious what it is and will check the patch and information shortly.

Comment 12

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

Comment on attachment 774022 [details] [diff] [review]
mozmill test patch

Review of attachment 774022 [details] [diff] [review]:
-----------------------------------------------------------------

No, this is not what we want. All shutdown and restart logic is located in frame.js. No code in controller should do that.

Comment 13

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

Comment on attachment 774017 [details] [diff] [review]
Patch v2

Review of attachment 774017 [details] [diff] [review]:
-----------------------------------------------------------------

As you have noted (really thanks for that!) there is a problem in Mozmill itself. I will file a separate bug on it in a bit.

Otherwise see my inline comments...

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +79,5 @@
>  
> +  // Bug 886811
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0
> +  // Revert to using startUserShutdown in mozmill-2.1

Please remove the line for Mozmill 2.1. This is not said, that 2.1 will fix it. I wouldn't do this claim.

::: tests/functional/restartTests/testAddons_changeTheme/test2.js
@@ +48,5 @@
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0
> +  // Revert to using startUserShutdown in mozmill-2.1
> +  if ("restartApplication" in controller) {
> +    controller.restartApplication();

Keep in mind that in the last test module for each restart test you should call stopApplication and reset the profile if necessary.

::: tests/functional/restartTests/testMenu_quitApplication/test1.js
@@ +21,5 @@
> +  }
> +  else {
> +    controller.startUserShutdown(4000, false);
> +    controller.mainMenu.click("#menu_FileQuitItem");
> +  }

Hm, I thought more about it, and lets get this reverted. There is no need to do any change in that file. But you might want to update the skip or manifest info.

::: tests/functional/restartTests/testRestartChangeArchitecture/test1.js
@@ +39,5 @@
>  /**
>   * Restart normally
>   */
>  function teardownModule(aModule) {
> +  var restartFlags = Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart;

There is no need to specify Ci.nsIAppStartup.eAttemptQuit and Ci.nsIAppStartup.eRestart. This is done internally by restartApplication.

@@ +46,5 @@
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0
> +  // Revert to using startUserShutdown in mozmill-2.1
> +  if ("restartApplication" in aModule.controller) {
> +    aModule.controller.restartApplication(null, false, restartFlags);

With my upcoming patch resetProfile will no longer be available for restartApplication().

@@ +55,4 @@
>  }
>  
>  if (persisted.skipTests) {
>    setupModule.__force_skip__ = "Architecture changes only supported on OSX 10.6";

Ups. Can you please update the message here so it says started with 10.6?

Comment 14

[Andrei Eftimie]

Attachment: patch v3

Thanks for your fix Henrik, OSX changeArchitecture tests work like a charm now!

For any reset profile action we're using stopApplication(true), added this too all last test for each restart testsuite.
Addressed feedback from the last comment.

Also some testruns on OSX. Will run them on Linux and Win if this approach holds.

Mozmill 1.5
===========
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc9475636ce


Mozmill 2.0
===========
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947568758

OSX (reference, without patch): http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc94757045b

Comment 15

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

Comment on attachment 775623 [details] [diff] [review]
patch v3

Review of attachment 775623 [details] [diff] [review]:
-----------------------------------------------------------------

Andrei, just a note that you please don't forget to ask Dave and myself for review in the future. Otherwise splitting up reviews will not work. Thanks.

Beside that you will also have to add the restart checks for l10n, and update tests. The latter might not work correctly otherwise, given that the setting of the updater log needs a restart.

::: tests/functional/restartTests/testRestartChangeArchitecture/test1.js
@@ +39,5 @@
>  /**
>   * Restart normally
>   */
>  function teardownModule(aModule) {
> +  var restartFlags = Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart;

I would not declare a variable here but move the value directly into the else case.

@@ +45,5 @@
> +  // Bug 886811
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0
> +  if ("restartApplication" in aModule.controller) {
> +    aModule.controller.restartApplication(null);

Remove 'null' as parameter. It's not necessary.

@@ +46,5 @@
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0
> +  if ("restartApplication" in aModule.controller) {
> +    aModule.controller.restartApplication(null);
> +  } else {

nit: We do not use an hanging else in our tests.

::: tests/functional/restartTests/testRestartChangeArchitecture/test2.js
@@ +25,5 @@
>   * Restart in 32 bit mode
>   */
>  function teardownModule(aModule) {
> +  var restartFlags = Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart |
> +                     Ci.nsIAppStartup.eRestarti386;

In mozmill 2.0 we only need 'Ci.nsIAppStartup.eRestarti386' so please not combine it in a single variable. The same applies to the following tests in that sub folder.

Comment 16

[Andrei Eftimie]

> Beside that you will also have to add the restart checks for l10n, and update
> tests. The latter might not work correctly otherwise, given that the setting
> of the updater log needs a restart.

Both l10n and update testruns are running without issue with mozmill-2.0
Where could I check the "updater log"?
I'm not sure. Do we restart between updates at the moment?
I have no issues with any l10n or update tests under 2.0

Comment 17

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

We discussed that today in the ask an expert meeting. So the answer has been given to Andrei.

Comment 18

[Andrei Eftimie]

Attachment: patch v4

Addressed review request and nits.

Both l10n and update tests are working fine on 2.0 without manual restart.
For better sandboxing I agree we should restart, and clean the profile between suites, so I've added that in.

The problem with the update log not showing in 2.0 appears unrelated to the restart issue. I've found no version of mozmill 2.0 that would show them. (Oldest version I've checked was from ~Oct 2012).

Filed bug 896475 to address that issue.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 779199 [details] [diff] [review]
patch v4

Review of attachment 779199 [details] [diff] [review]:
-----------------------------------------------------------------

Other than one nit about including a reference to a bug, this looks good to me. Could you also provide some dashboard reports?

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +78,5 @@
>    assert.equal(plainTheme.getNode().getAttribute("pending"), "enable");
>  
> +  // Bug 886811
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0

Could we include the appropriate bug number here (and elsewhere with this comment)?

Comment 21

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

Comment on attachment 779199 [details] [diff] [review]
patch v4

Review of attachment 779199 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +94,1 @@
>  }

This has to be placed into teardownModule. Otherwise we will not report a possible test failure. This applies to all the tests.

Beside that please lets try if user shutdown calls are more stable now with all the latest bug fixes in Mozmill.

::: tests/update/testDirectUpdate/test1.js
@@ +38,5 @@
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0
> +  if ("stopApplication" in aModule.controller) {
> +    aModule.controller.stopApplication();
> +  }

Why stopApplication() and not restartApplication()?

::: tests/update/testFallbackUpdate/test1.js
@@ +37,5 @@
> +  // Bug 886811
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // startUserShutdown is broken in mozmill-2.0
> +  if ("stopApplication" in aModule.controller) {
> +    aModule.controller.restartApplication();

stopApplication vs. restartApplication

Comment 22

[Andrei Eftimie]

Attachment: 5.patch

> This has to be placed into teardownModule. Otherwise we will not report a 
> possible test failure. This applies to all the tests.

Did this, had to use a 'hack' to get access to a needed variable from the test
proper in teardownModule

> Why stopApplication() and not restartApplication()?
Whoops. Fixed these.


Mozmill 1.5
===========
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61efda4c

Mozmill 2.0
===========
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61f769e2

Comment 23

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

Comment on attachment 781009 [details] [diff] [review]
5.patch

Review of attachment 781009 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +45,5 @@
>    // Store the theme in the persisted object
>    persisted.theme = THEME;
>  
> +  // We need acces to a variable from testInstallTheme in teardown
> +  aModule.temp = null;

The comment is superfluous if we would describe that variable better. Even a better name would not hurt, given that I cannot retrieve anything from 'temp'. We could also use persisted.

@@ +60,5 @@
> +  }
> +  else {
> +    // Restart the browser using restart prompt
> +    var restartLink = aModule.addonsManager.getElement({type: "listView_restartLink",
> +                                                        parent: aModule.temp});

We have to figure out a good strategy in the future how to handle that because it will fail if aModule.temp has not been set because of a test failure before the appropriate line. For now it is ok, given that Mozmill will restart the browser but for 2.0 we need the restartApplication() fallback.

@@ +96,5 @@
>    // Verify that plain-theme is marked to be enabled
>    assert.equal(plainTheme.getNode().getAttribute("pending"), "enable");
>  
> +  // We need acces to this addon in teardownModule
> +  temp = plainTheme;

Same as above. This needs to be made clearer.

::: tests/functional/restartTests/testAddons_changeTheme/test2.js
@@ +17,5 @@
>  
>    tabs.closeAllTabs(aModule.controller);
> +
> +  // We need acces to a variable from testThemeIsInstalled in teardown
> +  aModule.temp = null;

Same here.

::: tests/functional/restartTests/testAddons_enableDisableExtension/test2.js
@@ +53,4 @@
>  
> +    // User initiated restart
> +    controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true);
> +    controller.click(restartLink);

Why is this still not in teardownModule as I have mentioned in my last review? It seems like only a single test has been updated.

::: tests/functional/restartTests/testAddons_enableDisableExtension/test3.js
@@ +48,5 @@
>  
> +    // User initiated restart
> +    controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true);
> +    controller.click(restartLink);
> +  }

Same here and for any other test.

Comment 24

[Andrei Eftimie]

> We could also use persisted.

That was my initial try, but it didn't work.
My persisted object got wrecked.

I've investigated this more, and we have a bug in mozmill, see bug 906074
We should have that fixed! So I'm setting it as a blocker for this one, and we'll have these nicely stored in the persisted object.

Comment 25

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

What are you trying to store in the persisted object? It should not be an object which cannot be serialized!

Comment 26

[Andrei Eftimie]

We are storing reference to an addon, basically to a DOM node.
What this line returns:
http://hg.mozilla.org/qa/mozmill-tests/file/c9fdb8af06fc/lib/addons.js#l1075

Working fine with the patch I uploaded for bug 906074
We only serialize them between tests, and I agree that it looks better to use persisted.*

I'll have an updated patch soon.

Comment 27

[Andrei Eftimie]

Attachment: 6.patch

> The comment is superfluous if we would describe that variable better. Even a 
> better name would not hurt, given that I cannot retrieve anything from 'temp'.
> We could also use persisted.

Used persisted.installedAddon as a name, removed the comment.

> We have to figure out a good strategy in the future how to handle that because 
> it will fail if aModule.temp has not been set because of a test failure before 
> the appropriate line. For now it is ok, given that Mozmill will restart the 
> browser but for 2.0 we need the restartApplication() fallback.

In 2.0 we don't use the temporary variable anymore, we restart directly.
Should not be a problem...

> Why is this still not in teardownModule as I have mentioned in my last review?
> It seems like only a single test has been updated.

Whoa, big miss from me, I remember I thoroughly checked all files. But it has
been a while. I messed up somewhere.
Fixed now.

**This is now dependand on fixing the persisted object in bug 906074

Comment 28

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

Comment on attachment 792187 [details] [diff] [review]
6.patch

Review of attachment 792187 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testAddons_changeTheme/test2.js
@@ +27,5 @@
> +  }
> +  else {
> +    // Restart the browser using restart prompt
> +    var restartLink = aModule.addonsManager.getElement({type: "listView_restartLink",
> +                                                        parent: persisted.installedAddon});

Seeing the latest problems we have with the persisted object, I think that we really should not use this object for transferring data between tests in a single module. As more content we add as more data has to be send between the two ends. And over time with a lot of tests that will become a lot of data!

So it's probably wiser to really use a global variable in the module. Otherwise we could set the assigned object as null on the persisted object before we restart the browser. I think that path #1 sounds better.

Dave, what do you think?

Comment 29

[Andrei Eftimie]

Yeah, lets skip the persisted object for this. 
The hassle is not worth it.

Comment 30

[Andrei Eftimie]

Attachment: 7.patch

Lets hope we're good to land.
Reverted the changes to use persisted objects for storing DOM references into teardown into using the aModule namespace.

Mozmill 1.5 - all green
===========
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572eee6e04
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572eef8ebd

Mozmill 2.0 - failure reduction from 12 to 9
===========
POST-patch: http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ef0fb54
PRE-patch: http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ef1bbb6

Comment 31

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #28)
> Dave, what do you think?

Your first suggestion sounds good to me too.

Comment 32

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

Comment on attachment 794027 [details] [diff] [review]
7.patch

Review of attachment 794027 [details] [diff] [review]:
-----------------------------------------------------------------

One thing to be fixed here. Once done we can get this landed.

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +44,5 @@
>  
>    // Store the theme in the persisted object
>    persisted.theme = THEME;
>  
> +  aModule.persistInstalledAddon = null;

To lower the confusion with the persisted object, lets rename this variable to ´installedAddon'. Same applies to the other tests affected.

Comment 33

[Andrei Eftimie]

Attachment: 8.patch

> To lower the confusion with the persisted object, lets rename this variable to 
> ´installedAddon'. Same applies to the other tests affected.
Renamed variables

Running smoothly:
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19f9a30b
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19f961d5

Comment 34

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

Comment on attachment 796588 [details] [diff] [review]
8.patch

Review of attachment 796588 [details] [diff] [review]:
-----------------------------------------------------------------

One thing still needs to be fixed. With that you get my r+.

::: tests/functional/restartTests/testAddons_uninstallExtension/test4.js
@@ +54,5 @@
>                 "Extension '" + persisted.addons[0].id +
>                 "' was marked for uninstall");
>  
> +  // We need access to this addon in teardownModule
> +  aModule.installedAddon = enabledExtension;

aModule should not be used in a test function.

Comment 35

[Andrei Eftimie]

Attachment: 8.1.patch

> aModule should not be used in a test function.

Thanks for catching that!
I did check them all twice and apparently still missed it :(

Fixed!
Testrun still clean:
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19fd23bf
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19fbf0a4

Comment 36

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

Comment on attachment 796596 [details] [diff] [review]
8.1.patch

Review of attachment 796596 [details] [diff] [review]:
-----------------------------------------------------------------

You should take over r+ in the future. Also you have checkin permissions, so please land it yourself.

Comment 37

[Andrei Eftimie]

Huh, did not thought of doing that myself.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/2265039d64f1

Comment 38

[Andrei Eftimie]

Attachment: followup1.patch

Landed my own commit, and managed to mess it up.
Here is a quick fix for the messed update tests.

Only Update Tests are affected, but they all fail, so we should land this asap.

Comment 39

[Andrei Eftimie]

Here is how it is failing:
http://mozmill-daily.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a1031b36

With the follow-up applied:
http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a104e017

Comment 40

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 796623 [details] [diff] [review]
followup1.patch

Review of attachment 796623 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, although we have another bug for adding aModule.

Comment 41

[Andrei Eftimie]

Thanks Andrea for the quick turnaround.
Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/9adf6131f8f6

Comment 42

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

(In reply to Andrei Eftimie from comment #38)
> Landed my own commit, and managed to mess it up.
> Here is a quick fix for the messed update tests.
> 
> Only Update Tests are affected, but they all fail, so we should land this
> asap.

What went wrong here? I thought that it was deeply tested?

Also why again has this been marked as fixed and the flags not been updated? This is a crucial patch which we need for every branch. Please reconsider those things before closing bugs as fixed. Thanks!

Comment 43

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #42)
> What went wrong here? I thought that it was deeply tested?

I screwed up.
I focused on functional tests (and run at least 1 instance of them on both mozmill 1.5 and 2.0 after every small update to the patch). We had no problems there...

But apparently I didn't run the update tests after some of the later changes where I introduced the problem. 

> Also why again has this been marked as fixed and the flags not been updated?
> This is a crucial patch which we need for every branch. Please reconsider
> those things before closing bugs as fixed. Thanks!

Correct. We do need backports here.

Comment 44

[Andrei Eftimie]

Attachment: backport_aurora.patch

Comment 45

[Andrei Eftimie]

Attachment: backport_beta.patch

Comment 46

[Andrei Eftimie]

Attachment: backport_release.patch

Comment 47

[Andrei Eftimie]

Attachment: backport_esr17.patch

And some testruns and comments:

Mozmill 2.0
===========

Functional
----------
Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1171ba2
Beta: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1193b1c
Release: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a12c187e
ESR17: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a143a200
ESR17 is pretty broken in 2.0, not sure if we should do much to fix it.
Probably not worth it for its EOL is soon.

Update
------
Aurora: http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a118ff72
http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a1190c7c

Beta: http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a11bfc64
http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a11bf6bf

Mozmill 1.5
===========

Functional
----------
Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a117727f
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a11808b0

Beta: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a11a8507
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a11ae010

Release: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a131aa4b
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a131d411

ESR17: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1427888
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1424cc6

The failures on ESR17 seen here are also seen with a clean repo:
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a142b753

Seems a recent regression from mozmill-1.5.22, opened bug 911101 for it

Update
------
Aurora: http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a118ab2b
http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a1187d44

Beta: http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a11a4367
http://mozmill-crowd.blargon7.com/#/update/report/3c3cd991de01b704f3f65783a11a2928

Comment 48

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 797764 [details] [diff] [review]
backport_esr17.patch

Review of attachment 797764 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/db712e802c1e (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/af1a81a1de4f (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/52f4260b0be0 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/b8d947425159 (esr17)

Hope all is stable now. Thanks Andrei!

Comment 49

[Andrei Eftimie]

:( One problem still managed to slip on ESR17:
http://mozmill-daily.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace674451349be

I had to rename teardownTest into teardownModule in several tests (that change had not been backported into ESR17).

For 2 of the testRestartChangeArchitecture (which are only run on OSX) I missed also renaming the __force_skip__ statements. This makes these tests fail on Windows and Linux (where they are usually not run, but skipped).

Followup fix for the ESR17 branch will be up in a minute.

Comment 50

[Andrei Eftimie]

Attachment: esr17followupfix.patch

Followup for the mozilla-esr17 branch that fixes the 2 typos.

Testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace674451457ed

Comment 51

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 799394 [details] [diff] [review]
esr17followupfix.patch

Review of attachment 799394 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/52ef95b50cd9 (esr17)