Closed Bug 1296677 Opened 3 years ago Closed 3 years ago

Remove update billboard support from firefox-puppeteer

Categories

(Testing :: Firefox UI Tests, defect)

Version 3
defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: rstrong, Assigned: whimboo)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch for reference (obsolete) — Splinter Review
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 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.
Attachment #8782961 - Flags: review?(hskupin)
(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 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.
Attachment #8782961 - Flags: review?(hskupin) → review-
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.
Assignee: nobody → hskupin
(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?
Flags: needinfo?(sledru)
Note that we also have in tree tests for the channels that still have the code for the typical case of opening the billboard.
If it is that old, sure, happy that we remove this stuff.
Flags: needinfo?(sledru)
Ok, I will work on getting this patch finished (if other changes are necessary) and tested by tomorrow.
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.
Could you do that in a separate bug instead since this bug is holding up the landing of bug 1296207?
Attachment #8782961 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Summary: Remove app update billboard tests → Remove update billboard support from firefox-puppeteer
(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.
Attachment #8784258 - Flags: review?(robert.strong.bugs)
Comment on attachment 8784258 [details]
Bug 1296677 - Remove update billboard support from firefox-puppeteer.

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

Looks fine
Attachment #8784258 - Flags: review?(robert.strong.bugs) → review+
Thanks Rob, also for the pre-work to get this removed. The patch will be landed any moment on autoland now.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58862b93024b
Remove update billboard support from firefox-puppeteer. r=rstrong
https://hg.mozilla.org/mozilla-central/rev/58862b93024b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
You need to log in before you can comment on or make changes to this bug.