1005864 - Remove backward compatible shutdown handling for restart tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

Over on bug 886811 we got some lines added to our Mozmill restart tests to be able to handle both Mozmill 1.5 and 2.0. Given that we don't go back to the 1.5 branch we can finally remove all those lines:

   // Bug 886811
   // Mozmill 1.5 does not have the stopApplication method on the controller.
   // Remove condition when transitioned to 2.0
   if ("stopApplication" in aModule.controller) {
     aModule.controller.stopApplication(true);
   }

Comment 1

[Maria Grazia Alastra [:mga]]

Taken! This is my first automation-related bug.
The patch should be ready soon!

Comment 2

[Maria Grazia Alastra [:mga]]

Attachment: bug-1005864.patch

I only removed code that was there because of bug 886811, but there are also files with backwards-compatible code referencing bug 867217; let me know if I must remove those, too.

Obviously I ran all the restart tests after my modifications, they all passed without problems.

Comment 3

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

(In reply to Maria Grazia Alastra [:mga] from comment #1)
> Taken! This is my first automation-related bug.
> The patch should be ready soon!

Wonderful! Be welcome to automation then! :)

(In reply to Maria Grazia Alastra [:mga] from comment #2)
> I only removed code that was there because of bug 886811, but there are also
> files with backwards-compatible code referencing bug 867217; let me know if
> I must remove those, too.

Ah, good that you called that out. Yes, please also remove those extra code, as long as it is not related to user initiated shutdowns. Those are still not working properly.

Comment 4

[Andrei Eftimie]

Comment on attachment 8431153 [details] [diff] [review]
bug-1005864.patch

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

Yes, please also remove code that is referencing bug 867217.
Thanks Maria, otherwise it looks good.

Also when you run a testrun also supply the following argument --report=http://mozmill-crowd.blargon7.com/db/
And when you upload the new version of the patch please also include the url of the tesrun result.

Comment 5

[Maria Grazia Alastra [:mga]]

Attachment: bug-1005864-v2.patch

Removed code referencing bug 867217, too.
Report link: http://mozmill-crowd.blargon7.com/#/functional/report/4e9e123ff3e2bbcb949b2192929b2ae4

If I may, the "Notification popup visibility state has been changed" failure in /restartTests/testAddons_installMultipleExtensions/test1.js has occurred in a testrun done before taking this bug, I told Andreea Matei about that on the automation irc channel. 
It's also true that I couldn't replicate the failure in the next testruns.

Comment 6

[Andrei Eftimie]

Comment on attachment 8431466 [details] [diff] [review]
bug-1005864-v2.patch

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

> If I may, the "Notification popup visibility state has been changed" failure in /restartTests/testAddons_installMultipleExtensions/test1.js has occurred in a testrun done before taking this bug,
Yes, this is known and will be fixed in bug 994040.

Looks good, but you missed some tests from the `remote`, `update`, `l10n` testruns like:
> testAddons_InstallAddonWithoutEULA
> testAddons_installFromFTP
> testAddons_RestartlessExtensionWorksAfterRestart
> testDirectUpdate
> testFallbackUpdate
> testAccessKeys
> testCropped

Comment 7

[Maria Grazia Alastra [:mga]]

Attachment: bug-1005664-v3.patch

I should have checked all the tests, this time!

- Functional testrun: http://mozmill-crowd.blargon7.com/#/functional/report/076ed54106439c677bbfdee3201bd50d

- Remote testrun: http://mozmill-crowd.blargon7.com/#/remote/report/076ed54106439c677bbfdee3201c74e2

- L10n testrun: http://mozmill-crowd.blargon7.com/#/l10n/report/076ed54106439c677bbfdee3201e5d3d

- Update testrun: http://mozmill-crowd.blargon7.com/#/update/report/076ed54106439c677bbfdee3201da8ab

The update test failed completely, but I think it might be because of my internet connection. Right now I'm sharing connection with my smartphone (32Kbps), since the ADSL is broken and I'm waiting for technical assistance.

Let me know if there is something I must fix, or that I missed.
Thank you and sorry for the delay!

Comment 8

[Andrei Eftimie]

Comment on attachment 8433251 [details] [diff] [review]
bug-1005664-v3.patch

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

Looks good to me.

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/c4ecd47f1746 (default)

Comment 9

[Andrei Eftimie]

Thanks Maria for the work here.
We appreciate the help.

Let me know if you are interested in other bugs.

Comment 10

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

We also have to get those lines removed for the other branches. Otherwise we might run into major merge issues in the future. Also it should be clear that we don't use workarounds for mozmill 2.0.

Comment 11

[Maria Grazia Alastra [:mga]]

(In reply to Andrei Eftimie from comment #9)
> Let me know if you are interested in other bugs.

Of course I am! 


(In reply to Henrik Skupin (:whimboo) from comment #10)
> We also have to get those lines removed for the other branches. Otherwise we
> might run into major merge issues in the future. Also it should be clear
> that we don't use workarounds for mozmill 2.0.

Ok, so should I make a separate patch for each of the other branches or is it possible to merge the one already checked in?

Comment 12

[Andrei Eftimie]

(In reply to Maria Grazia Alastra [:mga] from comment #11)
> Ok, so should I make a separate patch for each of the other branches or is
> it possible to merge the one already checked in?

Yes, we will need that since the commit from default does not apply cleanly on the other branches.

Comment 13

[Maria Grazia Alastra [:mga]]

Attachment: bug-1005864-aurora.patch

Patch for the mozmill-aurora branch.

- Functional testrun: http://mozmill-crowd.blargon7.com/#/functional/report/076ed54106439c677bbfdee3205f4c83

- L10n testrun: http://mozmill-crowd.blargon7.com/#/l10n/report/076ed54106439c677bbfdee3205f5a67

- Remote testrun: http://mozmill-crowd.blargon7.com/#/remote/report/076ed54106439c677bbfdee3205fb7c1

- Update testrun: http://mozmill-crowd.blargon7.com/#/update/report/076ed54106439c677bbfdee320600358

Comment 14

[Maria Grazia Alastra [:mga]]

(In reply to Maria Grazia Alastra [:mga] from comment #13)
> Patch for the mozmill-aurora branch.

mozilla-aurora* branch.


Additional notes: the patch applies smoothly on mozilla-beta and mozilla-release, too.

Comment 15

[Maria Grazia Alastra [:mga]]

Attachment: bug-1005864-esr24.patch

Patch for mozilla-esr24 branch.

- Functional testrun: http://mozmill-crowd.blargon7.com/#/functional/report/076ed54106439c677bbfdee32069654e

- L10n testrun: http://mozmill-crowd.blargon7.com/#/l10n/report/076ed54106439c677bbfdee32069e8da

- Remote testrun: http://mozmill-crowd.blargon7.com/#/remote/report/076ed54106439c677bbfdee32069d8cb

- Update testrun: http://mozmill-crowd.blargon7.com/#/update/report/076ed54106439c677bbfdee3206a0a9b

Comment 16

[Andrei Eftimie]

Comment on attachment 8434181 [details] [diff] [review]
bug-1005864-esr24.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/7f3db19e858e (mozilla-aurora)
https://hg.mozilla.org/qa/mozmill-tests/rev/312c6425e079 (mozilla-beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/fe480d8db845 (mozilla-release)
https://hg.mozilla.org/qa/mozmill-tests/rev/639ee4bb4ea0 (mozilla-esr24)

Comment 17

[Andrei Eftimie]

Thanks again Maria.