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

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

mozmill-2.0 functional testrun does not restart between restartTests
This is the actual problem for bug 860670

Comment 1

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

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.

Comment 2

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

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.

Comment 3

[Andrei Eftimie]

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.

Comment 4

[Andrei Eftimie]

Attachment: 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

Comment 5

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

This is not a Mozmill issue but in our tests. Moving to the right component.

Comment 7

[Andrei Eftimie]

Attachment: 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.

Comment 8

[Andrei Eftimie]

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 9

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

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 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 749709 [details] [diff] [review]
patch v2

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

Based on Henrik's comment.

Comment 11

[Andrei Eftimie]

Attachment: patch v3

Added comments referencing this bug and reminding to remove the condition once we transition to mozmill 2.0

Comment 12

[Andreea Matei [:AndreeaMatei]]

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.

Comment 13

[Andrei Eftimie]

Attachment: 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.

Comment 14

[Andrei Eftimie]

Attachment: 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.

Comment 15

[Andreea Matei [:AndreeaMatei]]

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.

Comment 16

[Andrei Eftimie]

I have no problem running testruns under Mozmill 2.0:

OSX: http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c29dc4d7
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c2940afd

Comment 17

[Andrei Eftimie]

Attachment: 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

Comment 18

[Andreea Matei [:AndreeaMatei]]

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.

Comment 19

[Andrei Eftimie]

Attachment: 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

Comment 20

[Andreea Matei [:AndreeaMatei]]

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.

Comment 21

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

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.

Comment 22

[Andrei Eftimie]

Attachment: 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.

Comment 23

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

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.

Comment 24

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

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.

Comment 25

[Andrei Eftimie]

Attachment: Backport Aurora

Backport for Aurora

Comment 26

[Andrei Eftimie]

Attachment: Backport Beta

Comment 27

[Andrei Eftimie]

Attachment: Backport Release

Comment 28

[Andrei Eftimie]

Attachment: Backport ESR17

Comment 29

[Andrei Eftimie]

Attachment: Backport Testruns

Testruns for all backports

Comment 30

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

Release and beta are the same at the moment. Why do we need different patches?

Comment 31

[Andrei Eftimie]

mozilla-release and mozilla-beta are not the same at the moment.

Comment 32

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

What's different? I have merged beta into release 2 days ago and nothing else has been landed on beta yet. This is strange.

Comment 33

[Andrei Eftimie]

Attachment: 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.

Comment 34

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

Comment on attachment 765344 [details]
mozilla-release vs mozilla-beta

Yeah, this diff is before you did the last pull.

Comment 35

[Andreea Matei [:AndreeaMatei]]

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)

Comment 36

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

Updating summary so it's clear what has been done here without to confuse people with user shutdown modes.