1296677 - Remove update billboard support from firefox-puppeteer

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Robert Strong (they/them - no direct email)]

In bug 1296207 the app update billboard capability will be removed in preparation to rewriting the user interface. The Firefox UI Tests will need to be updated to not run the tests that exercise the billboard.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: Patch for reference

This patch is just for reference since it isn't clear that the tests can just be removed based on comments in the bug that the tests in m-c are used in esr.

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 8782961 [details] [diff] [review]
Patch for reference

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86ec7b2f150d

With the comment about esr in the code I'm not sure how you deal with other branches than the one this lands in. Asking for r+ on either getting this landed as is or to give you a heads up about the change if the tests need to be further modified.

Comment 3

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #0)
> In bug 1296207 the app update billboard capability will be removed in
> preparation to rewriting the user interface. The Firefox UI Tests will need
> to be updated to not run the tests that exercise the billboard.

Thanks for the heads-up. Do we already have a bug around which covers the UI rewrite? I would like to be prepared in case my help would be needed.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #2)
> With the comment about esr in the code I'm not sure how you deal with other
> branches than the one this lands in. Asking for r+ on either getting this
> landed as is or to give you a heads up about the change if the tests need to
> be further modified.

The update tests always have to be supported down 3 releases or at maximum the last ESR release. This is needed because at some point we have to update ESR-1 to current ESR. This also means lots of fallback code have to be kept around, and needs clean-up after ESR-1 got desupported.

Usually we should be fine to remove entries from puppeteer ui modules which no longer exists for mozilla-central, and are not used by any older version. If there is such a situation we need a version check and fallback code.

Comment 4

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

Comment on attachment 8782961 [details] [diff] [review]
Patch for reference

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

Rob, when do you target to land the removal of the billboard? I assume I would have to update this patch ASAP?

::: testing/firefox-ui/harness/firefox_ui_harness/testcases.py
@@ -286,5 @@
> -        # old update wizard dialog.
> -        if window.deck.selected_panel == window.deck.apply_billboard:
> -            dialog = self.browser.open_window(
> -                callback=lambda _: window.deck.update_button.click(),
> -                expected_window_class=UpdateWizardDialog

Do we remove the old UpdateWizard dialog, or won't we hit this code path anytime longer? If there is still a way to trigger the old software update dialog, we should keep this block, and update the if condition.

Also we would have to keep the block for now for older releases, which means it needs a version check. FYI if we do an update from 45.0ESR to eg. 51.0a we have to make use of the test package from the 51.0a build.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/about_window/deck.py
@@ -17,5 @@
>  
>          :returns: :class:`Panel` instance
>          """
>          mapping = {'apply': ApplyPanel,
> -                   'applyBillboard': ApplyBillboardPanel,

Both changes in this file we would have to keep, and as best mark to be removed once support for 45.0ESR ends.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/update_wizard/wizard.py
@@ -36,5 @@
>                     'manualUpdate': ManualUpdatePanel,
>                     'noupdatesfound': NoUpdatesFoundPanel,
>                     'pluginupdatesfound': PluginUpdatesFoundPanel,
>                     'updatesfoundbasic': UpdatesFoundBasicPanel,
> -                   'updatesfoundbillboard': UpdatesFoundBillboardPanel,

The same here for all of the changes.

Comment 5

[Robert Strong (they/them - no direct email)]

Answers to 
The bug for the rewrite is bug 893505.
I'd like to land the billboard removal yesterday. :)
There is no way to open the billboard from the about dialog once this lands hence the removal.

btw we haven't used billboards since 3.6 and there are no plans on using the billboard.

Comment 6

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #5)
> btw we haven't used billboards since 3.6 and there are no plans on using the
> billboard.

What could potentially break if there is a reason to use the billboard for an update of a build earlier than 51.0? I mean updates would not work because we hang and cannot process this unexpected new window. If we are 100% sure that we never will use a billboard, I'm fine with removing all that code. 

Sylvestre, maybe you can give a go for that?

Comment 7

[Robert Strong (they/them - no direct email)]

Note that we also have in tree tests for the channels that still have the code for the typical case of opening the billboard.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

If it is that old, sure, happy that we remove this stuff.

Comment 9

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

Ok, I will work on getting this patch finished (if other changes are necessary) and tested by tomorrow.

Comment 10

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

While checking this patch locally I noticed that we are not running the test_update_wizard.py test because it is not listed in the manifest. :S I will have to update this test and the puppeteer ui module to bring the code up-to-date with the update.xul dialog. I hope it will be fine to include all of that in this patch.

Comment 11

[Robert Strong (they/them - no direct email)]

Could you do that in a separate bug instead since this bug is holding up the landing of bug 1296207?

Comment 12

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

Attachment: Bug 1296677 - Remove update billboard support from firefox-puppeteer.

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

Comment 13

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #11)
> Could you do that in a separate bug instead since this bug is holding up the
> landing of bug 1296207?

It's already all done and pass locally. I just triggered a try build. In case you want to already review my patch, I will add the flag now.

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 8784258 [details]
Bug 1296677 - Remove update billboard support from firefox-puppeteer.

https://reviewboard.mozilla.org/r/73776/#review71638

Looks fine

Comment 15

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

Thanks Rob, also for the pre-work to get this removed. The patch will be landed any moment on autoland now.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58862b93024b
Remove update billboard support from firefox-puppeteer. r=rstrong

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58862b93024b

Comment 18

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

Re-enabling the test_appinfo.py test via this patch revealed an underlying Toolkit bug which reports the wrong platformBuildID. See bug 1298328. I will go ahead and just re-enable the test for aurora too, without backporting the whole patch here.