786252 - "Profile already in use" message after /restartTests/testMenu_quitApplication causes profile to remain

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Alex Lakatos[:AlexLakatos]]

We should skip this until bug 785602 is fixed or we move on to Mozmill 2.0

Comment 1

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

Skipping the test in test1.js and in manifest.ini

Comment 2

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

This is not a failure in general but the framework behaves badly and so it affects the whole testrun because the used profile for this test will stay around and is not getting deleted. So marking as P1.

Comment 3

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

Comment on attachment 655982 [details] [diff] [review]
patch v1.0

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

::: tests/functional/restartTests/manifest.ini
@@ +9,5 @@
>  [include:testAddons_uninstallExtension/manifest.ini]
>  [include:testAddons_uninstallTheme/manifest.ini]
>  [include:testDefaultBookmarks/manifest.ini]
>  [include:testMenu_quitApplication/manifest.ini]
> +disabled = Bug 786252 - mozmill bug with restart of application

I have updated the summary of the bug so it says why we have to disable this test. Please update to it is more descriptive. The current summary will not tell you anything why it was necessary.

::: tests/functional/restartTests/testMenu_quitApplication/test1.js
@@ +17,5 @@
> +// Bug 786252 - Skip /restartTests/testMenu_quitApplication
> +// because of mozmill bug with restart of application
> +setupModule.__force_skip__ = "Bug 786252 - Skip /restartTests/testMenu_quitApplication " +
> +                             "because of mozmill bug with restart of application";
> +

Same here and also remove the comment. It's just duplication of content. There is also no need to reference the test name in the skip string. Further put this at the very end of the test.

Comment 4

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Same here and also remove the comment. It's just duplication of content.
> There is also no need to reference the test name in the skip string. Further
> put this at the very end of the test.

We're not being consistent here. Every test has the comment although just on this one the skip block is not at the very end. We should file a new bug for skip block cleanup, if our policy changed, for consistency.
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testSecurity/testSafeBrowsingWarningPages.js

Comment 5

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

Comment on attachment 656381 [details] [diff] [review]
patch v2.0

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

(In reply to Alex Lakatos[:AlexLakatos] from comment #4)
> We're not being consistent here. Every test has the comment although just on
> this one the skip block is not at the very end. We should file a new bug for
> skip block cleanup, if our policy changed, for consistency.

I know that. But I don't see a reason why we have to waste our time on those changes. These are polish things and we already have started to avoid to add those unnecessary duplicated content.

::: tests/functional/restartTests/testMenu_quitApplication/test1.js
@@ +19,5 @@
>   */
>  testQuitApplication.meta = {moztrap_case: 333};
> +
> +setupModule.__force_skip__ = "Bug 786252 - 'Profile already in use' message after " +
> +                             "/restartTests/testMenu_quitApplication causes profile to remain";

Please use the same message as in manifest.ini.

Comment 6

[Alex Lakatos[:AlexLakatos]]

Attachment: skip patch v3 [checked-in]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> I know that. But I don't see a reason why we have to waste our time on those
> changes. These are polish things and we already have started to avoid to add
> those unnecessary duplicated content.
I'm ok with leaving things like this for the moment. But I'm looking at things from the point of view of a new contributor: It's hard to pick things up if there is no consistency, no immediate rule that pops out from our code. Maybe we should consider making this a P3 or good-first-bug? I'm ok with mentoring it as well if so.

Comment 7

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

Comment on attachment 656447 [details] [diff] [review]
skip patch v3 [checked-in]

Skipped test:
http://hg.mozilla.org/qa/mozmill-tests/rev/d20382729bdd (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/a692a079e8cb (aurora)

Comment 8

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

Clearing priority given that we are blocked on the Mozmill 2.0 release.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Giving that we already have Mozmill 2.0, I checked this and it's all good on default. I can backout the skip on default and do the other branches tomorrow.

http://mozmill-crowd.blargon7.com/#/functional/report/5c334d437fbb9f11a2de1d989b230f33
http://mozmill-crowd.blargon7.com/#/functional/report/5c334d437fbb9f11a2de1d989b231a9a
http://mozmill-crowd.blargon7.com/#/functional/report/0906e2c8b6f223d6588987bcd1000d47

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

Comment 10

[Andreea Matei [:AndreeaMatei]]

Aurora was reenabled due to the merge.

http://hg.mozilla.org/qa/mozmill-tests/rev/60a364e53a4b (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/ae8c0d3024aa (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/223aa4873b0f (esr24)