restart tests have to call controller.restartApplication() for mozmill 2.0 compatibility

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: andrei, Assigned: andrei)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox22 fixed, firefox23 fixed, firefox24 fixed, firefox25 fixed, firefox-esr17 fixed)

Details

(Whiteboard: [mozmill-2.0-compat][sprint2013-29])

Attachments

(6 attachments, 8 obsolete attachments)

(Assignee)

Description

6 years ago
mozmill-2.0 functional testrun does not restart between restartTests
This is the actual problem for bug 860670
Mozmill 2.0 does not do restart tests in the same way. As I understand it, these will all be refactored for Mozmill 2.0 compatibility.
Please never file something as blocking mozmill-2.0. All bugs have to go through the triage process. As we have discussed in one of our first triage meetings any restart test is not in our focus for now. So it may not block 2.0. But we will see. Also I think it may be a dupe of a bug which is already on file.
Whiteboard: [mozmill-2.0+] → [mozmill-2.0?]
(Assignee)

Comment 3

6 years ago
Sorry for that. Filed it as blocking for mozmill-2.0 because it blocks 860670 (and that is set to block mozmill-2.0).

Will bring these up in today's meeting.
(Assignee)

Updated

6 years ago
Summary: testrun_functional does not restart between restartTests → restart tests should manually restart for mozmill 2.0 compatibility
(Assignee)

Comment 4

6 years ago
Created attachment 745193 [details] [diff] [review]
Patch v1

Changed restart tests to manually restart if we have that option (mozmill 2.0)
Didn't ask for review since I couldn't get a positive run on Linux with mozmill 1.5 (reports and error messages below)

Patch not applied - reference
=============================

Mozmill 2.0
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa065973ed

Mozmill 1.5
Linux:
TEST-PASS | /tmp/tmp5gR6xc.mozmill-tests/tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension/test2.js | test2.js::testBlocklistsExtension
Sorry, cannot connect to jsbridge extension, port 24242
*** Removing repository '/tmp/tmp5gR6xc.mozmill-tests'

Patch applied
=============

Mozmill 2.0
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa06576b83
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa0686124d
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa068f36e9

Mozmill 1.5
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa0655a641
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/452ec32f8deec0960aea87aa06863f41
Linux. No testrun
TEST-PASS | /tmp/tmp8BgEXs.mozmill-tests/tests/functional/testInstallation/testBreakpadInstalled.js | testBreakpadInstalled.js::testBreakpadInstalled
NOTE: child process received `Goodbye', closing down
WARNING: waitpid failed pid:3696 errno:10: file /builds/slave/m-cen-lx-ntly-0000000000000000/build/ipc/chromium/src/base/process_util_posix.cc, line 260
WARNING: waitpid failed pid:3696 errno:10: file /builds/slave/m-cen-lx-ntly-0000000000000000/build/ipc/chromium/src/base/process_util_posix.cc, line 260
WARNING: Failed to deliver SIGKILL to 3696!(3).: file /builds/slave/m-cen-lx-ntly-0000000000000000/build/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118
INFO Passed: 210
INFO Failed: 1
INFO Skipped: 13
Report document created at 'http://mozmill-crowd.blargon7.com/db/452ec32f8deec0960aea87aa06906616'
Sorry, cannot connect to jsbridge extension, port 24242
This is not a Mozmill issue but in our tests. Moving to the right component.
No longer blocks: 860670
Component: Mozmill → Mozmill Tests
Product: Testing → Mozilla QA
Whiteboard: [mozmill-2.0?]
Priority: -- → P2
Whiteboard: [mozmill-2.0-compat]
Duplicate of this bug: 860670

Updated

5 years ago
Whiteboard: [mozmill-2.0-compat] → [mozmill-2.0-compat][sprint2013-29]
(Assignee)

Comment 7

5 years ago
Created attachment 749709 [details] [diff] [review]
patch v2

1. We manually restart between each restart test and
2. We also reset the profile between each restart suits


Mozmill 1.5
-----------
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c20b1c1f
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c210cd4b
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c2278a1e

Mozmill 2.0
-----------
OSX: http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145ee27b5e
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c2167318

** I'm having a hard time getting a complete functional testrun for mozmill 2.0 under windows, keep getting "IO Completion Port unexpectedly closed" which seem to lead to a Application disconnect error. Raised bug 872414 for that.
Attachment #745193 - Attachment is obsolete: true
Attachment #749709 - Flags: review?(andreea.matei)
(Assignee)

Comment 8

5 years ago
Oh, forgot to add one additional note:

I had to also skip the teardown in 
> testAddons_uninstallExtension/test5.js 
which was skipped for bug 783484 (but wrongfully I missed the teardown skip there)

And it would cras: teardown would ran; since we need the controller which is instantiated in setup which is skipped.

I've added that here. Maybe should have a different patch for that in bug 783484 ?
Comment on attachment 749709 [details] [diff] [review]
patch v2

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

::: tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test1.js
@@ +68,5 @@
>    var addonIsInstalled = addonsManager.isAddonInstalled({addon: anAddon});
>  
>    assert.ok(addonIsInstalled, ADDON.id + " is successfully installed");
> +
> +  if ("restartApplication" in controller)

Please add a comment to all of those cases which says why we need this if condition. Also reference this bug. We should remove it once transitioned to Mozmill 2.0.
Comment on attachment 749709 [details] [diff] [review]
patch v2

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

Based on Henrik's comment.
Attachment #749709 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 749839 [details] [diff] [review]
patch v3

Added comments referencing this bug and reminding to remove the condition once we transition to mozmill 2.0
Attachment #749709 - Attachment is obsolete: true
Attachment #749839 - Flags: review?(andreea.matei)
Comment on attachment 749839 [details] [diff] [review]
patch v3

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

We're getting closer, one more change needed.

::: tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test1.js
@@ +68,5 @@
>    var addonIsInstalled = addonsManager.isAddonInstalled({addon: anAddon});
>  
>    assert.ok(addonIsInstalled, ADDON.id + " is successfully installed");
> +
> +  // XXX Bug 867217: Mozmill 1.5 does not have the restartApplication method

We no longer use XXX and now we also have a bug to reference. Also please move the description below the bug no.
Attachment #749839 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 750303 [details] [diff] [review]
patch v4

Changed the comment as asked.

*As a sidenote I opened bug 872918 since these comments are not consistent and we should fix that.
Attachment #749839 - Attachment is obsolete: true
Attachment #750303 - Flags: review?(andreea.matei)
(Assignee)

Comment 14

5 years ago
Created attachment 750419 [details] [diff] [review]
patch 5

Fixed a problem in testAddons_uninstallExtension.
Since tests 4 and 5 are skipped we need to reset the profile in test3.
Also added a comment to not forget to remove that when we unskip the last 2 tests.
Attachment #750303 - Attachment is obsolete: true
Attachment #750303 - Flags: review?(andreea.matei)
Attachment #750419 - Flags: review?(andreea.matei)
No longer blocks: 860669
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 750419 [details] [diff] [review]
patch 5

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

There is still something wrong with, I keep getting this window with "Firefox is already running, but is not responding.. ", when ran with 2.0, after different tests (is not one specific that creates this issue). Can you further investigate? We can't have this error as it is blocking the testrun to continue.
Attachment #750419 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 752701 [details] [diff] [review]
patch v6

Updated patch (would not apply cleanly anymore).

I did manage to have a failed run (out of about 6), as you reported: we get a message that a Firefox instance is already running, then we hang with a Disconnect Error.
Opened bug 874856 for that

However that doesn't seem related to this bug, since most runs don't fail that way.

More testruns:

Linux (Ubuntu 12.04):
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b442d4247
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b442d76e9

OSX:
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b442d7372
Attachment #750419 - Attachment is obsolete: true
Attachment #752701 - Flags: review?(andreea.matei)
Comment on attachment 752701 [details] [diff] [review]
patch v6

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

This is no longer applying cleanly and also I get this failure on both 1.5 and 2.0, while with a clean repository that test passes:
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b44e96eb9

Please add reports when uploading the new patch.

::: tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test1.js
@@ +73,5 @@
> +
> +  // Bug 867217
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // Remove condition when transitioned to 2.0
> +  if ("restartApplication" in controller)

Please add brackets to all files.
Attachment #752701 - Flags: review?(andreea.matei) → review-
(Assignee)

Updated

5 years ago
Depends on: 877101
(Assignee)

Comment 19

5 years ago
Created attachment 755317 [details] [diff] [review]
patch v7

Added curly braces for all if clauses.
Out guide recommends omitting them for 1 liners if its still readable: https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Conditionals
We should probably update that if it no longer holds.

Also each restart call lives now in teardownModule(), add the teardownModule function where there wasn't one just for this call. Makes code more cleaner to have this consistent.

Have no Linux testruns for 2.0 because of bug 877101

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

OSX: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b05623929160ca84e
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b05623929160d2e9c

Windows: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b05623929161aeb65
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b05623929161b50f7

Linux: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916133fd6
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916134e9a

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

OSX: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b05623929160fc7ec
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b05623929161ae365
Attachment #752701 - Attachment is obsolete: true
Attachment #755317 - Flags: review?(andreea.matei)

Updated

5 years ago
Blocks: 744007
(Assignee)

Updated

5 years ago
Blocks: 860671
Comment on attachment 755317 [details] [diff] [review]
patch v7

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/b8a22c8c9a3b (default)

Lets see how our testrun looks like in 2.0 since this and select() have been fixed.
Attachment #755317 - Flags: review?(andreea.matei) → review+
status-firefox24: --- → fixed
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox-esr17: --- → affected
Comment on attachment 755317 [details] [diff] [review]
patch v7

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

::: tests/functional/restartTests/testSoftwareUpdateAutoProxy/test1.js
@@ +31,5 @@
> +  // Bug 867217
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // Remove condition when transitioned to 2.0
> +  if ("restartApplication" in aModule.controller) {
> +    aModule.controller.restartApplication(null, true);

This should not reset the profile here! This will cause the test to misbehave. Please re-check all of the instances before we get this backported. A follow-up is necessary here.
(Assignee)

Comment 22

5 years ago
Created attachment 763458 [details] [diff] [review]
Followup patch 1

It is really weird how this regressed.
I could not find any instance of the regressed code in any locally saved patches.

I have no clue on how I managed to botch that.

Anyway here is a followup patch which fixes the problem.
Also rechecked all restartTests to make sure no other regressions are found.
Attachment #755317 - Attachment is obsolete: true
Attachment #763458 - Flags: review?(andreea.matei)
(Assignee)

Updated

5 years ago
Attachment #755317 - Attachment is obsolete: false
Comment on attachment 763458 [details] [diff] [review]
Followup patch 1

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

I need this test for my local io port testing, so I would like to land it now.
Attachment #763458 - Flags: review?(andreea.matei) → review+
Attachment #755317 - Flags: checkin+
Comment on attachment 763458 [details] [diff] [review]
Followup patch 1

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

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/cf49310c4232

Please check the other branches, so that we can get this backported.
Attachment #763458 - Flags: checkin+
(Assignee)

Comment 25

5 years ago
Created attachment 765259 [details] [diff] [review]
Backport Aurora

Backport for Aurora
Attachment #765259 - Flags: review?(andreea.matei)
(Assignee)

Comment 26

5 years ago
Created attachment 765261 [details] [diff] [review]
Backport Beta
Attachment #765261 - Flags: review?(andreea.matei)
(Assignee)

Comment 27

5 years ago
Created attachment 765262 [details] [diff] [review]
Backport Release
Attachment #765262 - Flags: review?(andreea.matei)
(Assignee)

Comment 28

5 years ago
Created attachment 765264 [details] [diff] [review]
Backport ESR17
Attachment #765264 - Flags: review?(andreea.matei)
(Assignee)

Comment 29

5 years ago
Created attachment 765265 [details]
Backport Testruns

Testruns for all backports
Release and beta are the same at the moment. Why do we need different patches?
(Assignee)

Comment 31

5 years ago
mozilla-release and mozilla-beta are not the same at the moment.
What's different? I have merged beta into release 2 days ago and nothing else has been landed on beta yet. This is strange.
(Assignee)

Comment 33

5 years ago
Created attachment 765344 [details]
mozilla-release vs mozilla-beta

So the merge has already been made?

Maybe I am misunderstanding how this should work.
Here is a diff between 2 branches: mozilla-release vs mozilla-beta

Anyway, retested now and the patch for beta now applies cleanly on the release branch. I might have started working on them right before you did the merge.

Marking that Release patch as obsolete.
Attachment #765262 - Attachment is obsolete: true
Attachment #765262 - Flags: review?(andreea.matei)
Attachment #765344 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 765344 [details]
mozilla-release vs mozilla-beta

Yeah, this diff is before you did the last pull.
Attachment #765344 - Attachment is obsolete: true
Comment on attachment 765259 [details] [diff] [review]
Backport Aurora

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

Due to the merges, aurora patch is now on beta:
http://hg.mozilla.org/qa/mozmill-tests/rev/58861ba75b0a (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/50710a8ef5f5 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/8521d0f4c211 (esr17)
Attachment #765259 - Flags: review?(andreea.matei) → review+
Attachment #765261 - Flags: review?(andreea.matei) → review+
Attachment #765264 - Flags: review?(andreea.matei) → review+
Updating summary so it's clear what has been done here without to confuse people with user shutdown modes.
Summary: restart tests should manually restart for mozmill 2.0 compatibility → restart tests have to call controller.restartApplication() for mozmill 2.0 compatibility
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox21: affected → ---
status-firefox22: affected → fixed
status-firefox23: affected → fixed
status-firefox25: --- → fixed
status-firefox-esr17: affected → fixed
Resolution: --- → FIXED
Depends on: 1005864
You need to log in before you can comment on or make changes to this bug.