Closed Bug 1005864 Opened 6 years ago Closed 6 years ago

Remove backward compatible shutdown handling for restart tests

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(firefox30 fixed, firefox31 fixed, firefox32 fixed, firefox-esr24 fixed)

RESOLVED FIXED
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
firefox32 --- fixed
firefox-esr24 --- fixed

People

(Reporter: whimboo, Assigned: mga)

References

()

Details

(Whiteboard: [mentor=andrei.eftimie][lang=js][good first bug])

Attachments

(3 files, 2 obsolete files)

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);
   }
Blocks: 886811
Taken! This is my first automation-related bug.
The patch should be ready soon!
Assignee: nobody → alastra.mariagrazia
Attached patch bug-1005864.patch (obsolete) — Splinter Review
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.
Attachment #8431153 - Flags: review?(hskupin)
Attachment #8431153 - Flags: review?(andrei.eftimie)
(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.
Blocks: 867217
Status: NEW → ASSIGNED
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.
Attachment #8431153 - Flags: review?(hskupin)
Attachment #8431153 - Flags: review?(andrei.eftimie)
Attachment #8431153 - Flags: review-
Attached patch bug-1005864-v2.patch (obsolete) — Splinter Review
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.
Attachment #8431153 - Attachment is obsolete: true
Attachment #8431466 - Flags: review?(hskupin)
Attachment #8431466 - Flags: review?(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
Attachment #8431466 - Flags: review?(hskupin)
Attachment #8431466 - Flags: review?(andrei.eftimie)
Attachment #8431466 - Flags: review-
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!
Attachment #8431466 - Attachment is obsolete: true
Attachment #8433251 - Flags: review?(hskupin)
Attachment #8433251 - Flags: review?(andrei.eftimie)
Attachment #8433251 - Flags: review?(hskupin)
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)
Attachment #8433251 - Flags: review?(andrei.eftimie)
Attachment #8433251 - Flags: review+
Attachment #8433251 - Flags: checkin+
Thanks Maria for the work here.
We appreciate the help.

Let me know if you are interested in other bugs.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
(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.
(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.
Attachment #8434113 - Flags: review?(andrei.eftimie)
Attachment #8434113 - Flags: review+
Attachment #8434113 - Flags: checkin+
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)
Attachment #8434181 - Flags: review?(andrei.eftimie)
Attachment #8434181 - Flags: review+
Attachment #8434181 - Flags: checkin+
Thanks again Maria.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.