1317164 - browser/components/customizableui/test/browser_967000_button_sync.js fails to run more than once in same browser session

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Vishnu (:malayaleecoder)]

./mach mochitest browser/components/customizableui/test/browser_967000_button_sync.js --repeat 1 fails. This should not happen with our tests :)

Comment 1

[Vishnu (:malayaleecoder)]

Attachment: Bug1317164_v1.diff

I am not sure whether this will break the purpose of the test. But it solves the multiple run failure issue.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8810210 [details] [diff] [review]
Bug1317164_v1.diff

redirecting to :markh, I am not familiar with this intentions of this test and possibly Mark will know or can help find someone.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8810210 [details] [diff] [review]
Bug1317164_v1.diff

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

::: browser/components/customizableui/test/browser_967000_button_sync.js
@@ +117,5 @@
>  }
>  
>  // When Sync is not setup.
>  add_task(() => openPrefsFromMenuPanel("PanelUI-remotetabs-setupsync", "synced-tabs"));
> +document.getElementById("PanelUI-remotetabs-setupsync").hidden = false;

So IIUC, this will show the panel as the test starts up - which if that is really the intent, it should probably be moved to the top of the test. However, I think it would be better to have openPrefsFromMenuPanel() do this to expectedPanelId() as it is finished.

Comment 4

[Vishnu (:malayaleecoder)]

Attachment: Bug1317164_v2.diff

Please have a look, I have incorporated the above said changes :)

Comment 5

[Vishnu (:malayaleecoder)]

Attachment: Single run in same browser session

Comment 6

[Vishnu (:malayaleecoder)]

Attachment: Multiple runs in same browser session

Comment 7

[Vishnu (:malayaleecoder)]

The above attached two screenshots were captured nearing the completion of the test runs. As you can notice, while running multiple times, there is a pop up of a SVG image (sync-illustration.svg) along with Sync options. Not sure how/why this is happening, just wanted to bring to notice.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8812860 [details] [diff] [review]
Bug1317164_v2.diff

I don't think that's quite right either, but I had a play and worked out what was :)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1317164 - ensure browser_967000_button_sync.js cleans up after itself completely.

Review commit: https://reviewboard.mozilla.org/r/95576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95576/

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Vishnu, can you make sure this runs fine on all of your tests locally and see if the code makes sense to you and r+ if so?

Comment 11

[Vishnu (:malayaleecoder)]

Comment on attachment 8814310 [details]
Bug 1317164 - ensure browser_967000_button_sync.js cleans up after itself completely.

https://reviewboard.mozilla.org/r/95576/#review97180

It's all good

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ea964e7ab59
ensure browser_967000_button_sync.js cleans up after itself completely. r=malayaleecoder

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ea964e7ab59