844865 - Update library modules to be ready for the removal of Panorama

Status: RESOLVED WONTFIX

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

Description

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

Based on the work on bug 836758 our tests will be broken soon given that some of the lookup strings we make use of in our libs can't be resolved anymore. We should have a patch ready to land whenever the patch on bug 836758 lands on central.

This has to be our P1 this week and needs to be fixed ASAP.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to Henrik Skupin (:whimboo) from comment #0)
> This has to be our P1 this week and needs to be fixed ASAP.

I don't think bug 836758 will be landing this week, we still have to figure out what the transition plan will be.

Comment 2

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> I don't think bug 836758 will be landing this week, we still have to figure
> out what the transition plan will be.

We want to be prepared for this change and do not have to suffer from failures for 1 or 2 weeks if we weren't able to land our changes in time. Having a patch ready we can land alongside the patch on bug 836758 will make our test results stay green. So once ready we will wait for sure, it's not a problem for us when Panorama gets removed.

Comment 3

[Andrei Eftimie]

Attachment: Part 1 - remove tabView functionality

Part 1 - removing all references to tabView (panorama)
This patch will have our testrun working correctly after the tabview
functionality is taken out of FF
Functional testrun: http://mozmill-crowd.blargon7.com/#/functional/report/66aad0ebfa7a01580628b3af4c139c6f
Endurance testrun: http://mozmill-crowd.blargon7.com/#/endurance/report/66aad0ebfa7a01580628b3af4c13e47a

*These testruns are on the latests Nightly Builds (which still have tabView
funct). The patches do not longer apply cleanly, so its a bit more work to get a 
build with the funct removed. I am about 95% sure this will work, but we should
have a testrun on a build with these removed. working on this.

** a Part 2 will follow which will reenable our functional tests as addon tests
for the new Addon that assumes the removed functionality.

Comment 4

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

I don't think we should remove the lib but move it into the addons folder given that we want to run add-on tests for the new addon. That will keep us the history of the file.

Comment 5

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> I don't think we should remove the lib but move it into the addons folder
> given that we want to run add-on tests for the new addon. That will keep us
> the history of the file.

I agree with keeping the history for the file.
We will not be able to have a complete Addon test until the addon is finished and available.
I will leave patches in 2 parts so we can quickly disable these tests when the tabView removal lands, but I will work on having them merged. Hopefully we can have them ready at the same time. (Maybe we should just skip the tests if we need to disable them and don't have the Addons part ready).

I now have a build with the tabView removed, and the Part 1 patch is working fine:

# FF build with tabView removed

## unpatched testsuite
functional: http://mozmill-crowd.blargon7.com/#/functional/report/66aad0ebfa7a01580628b3af4c176da8
endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/66aad0ebfa7a01580628b3af4c1a07f8

## patched testsuite
functional: http://mozmill-crowd.blargon7.com/#/functional/report/66aad0ebfa7a01580628b3af4c1c4a14
endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/66aad0ebfa7a01580628b3af4c1ae8c7

Working on getting the Addons patch ready.

One thing that we will need is a download location for the xpi.

Comment 6

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

Tim could most likely point us to a current version of the addon we could use for testing purposes.

Comment 7

[Andrei Eftimie]

Attachment: moveTabViewTestToAddon_v1

It seems I was underconfident: here is a fully working patch.
We are removing the endurance tests, and migrating the functional tests to an Addon suite.

For an addons testrun to pass, you will need a patched build of FF with the tabView functionality removed. (It will fail with the latest Nightly)

I am only asking for feedback here because we still need the canonical location of the xpi. ATM I have forked the addon repo, added the xpi and linked it directly from github.

Comment 8

[Andrei Eftimie]

*Note*
We might use a local Addon for this, but it will require someone to maintain/update it regularly.
I would prefer to use the canonical xpi location (I am guessing it will eventually be in addons.mozilla.org)
We can also land this patch as-is, as the xpi is hosted on github atm, but we will need to update the location later on.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Tim could most likely point us to a current version of the addon we could
> use for testing purposes.

Here it is: https://github.com/ttaubert/firefox-tabgroups

This add-on will interfere with the current Panorama if you add it to a build without the patches that remove Panorama.

Comment 10

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

Comment on attachment 719046 [details] [diff] [review]
moveTabViewTestToAddon_v1

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

Looks fine. Please address the items I have mentioned.

::: tests/addons/[e-mail]/addon.ini
@@ +1,4 @@
> +[download]
> +linux=https://github.com/andreieftimie/firefox-tabgroups/blob/master/firefox-tabgroups.xpi?raw=true
> +mac=https://github.com/andreieftimie/firefox-tabgroups/blob/master/firefox-tabgroups.xpi?raw=true
> +win=https://github.com/andreieftimie/firefox-tabgroups/blob/master/firefox-tabgroups.xpi?raw=true

Please update to the link Tim has given. Once it's on AMO we will use the final URL.

::: lib/tabview.js
@@ +432,5 @@
>     *
>     * @returns Array of external DTD urls
>     * @type [string]
>     */
>    getDtds : function tabView_getDtds() {

Hm, any reason why we have a getter called 'dtds' and this method in the same class?

::: tests/addons/[e-mail]/manifest.ini
@@ +1,1 @@
> +[include:tests/manifest.ini]

I miss this manifest in the patch. Also you want to remove it from the functional tests manifest.

Comment 11

[Andrei Eftimie]

Attachment: moveTabViewTestToAddon_v2

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Please update to the link Tim has given. Once it's on AMO we will use the
> final URL.

Tim's repo does not contain the xpi file.
I will ask Tim to see if he'll add the xpi to the repo so we can use it directly

> ::: lib/tabview.js
> @@ +432,5 @@
> >     *
> >     * @returns Array of external DTD urls
> >     * @type [string]
> >     */
> >    getDtds : function tabView_getDtds() {
> 
> Hm, any reason why we have a getter called 'dtds' and this method in the
> same class?

I have not questioned the library code.
It does seem weird as the getter and the getDtd method do not return the same
thing.

> ::: tests/addons/[e-mail]/manifest.ini
> @@ +1,1 @@
> > +[include:tests/manifest.ini]
> 
> I miss this manifest in the patch. Also you want to remove it from the
> functional tests manifest.

1. I have removed the orphaned manifest include.
2. The "tests/manifest.ini" file is in the patch (check line 143 and 144).
You might have missed it because it is just a rename.

Comment 12

[Andrei Eftimie]

Tim, would you please add the firefox-tabgroups addon xpi file to the github repo.

We need a location from which to download and install it directly from our test suitel and untill it is published on AMO this is probably the best location to serve it from.

Thanks

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Andrei Eftimie from comment #12)
> Tim, would you please add the firefox-tabgroups addon xpi file to the github
> repo.

Done.

Comment 14

[Andrei Eftimie]

Attachment: moveTabViewTestToAddon_v3

Added correct xpi download location.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 719925 [details] [diff] [review]
moveTabViewTestToAddon_v3

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

Looks good. 
I feel though that getter get dtds() and method detDtds in tabview.js class are really strange, giving that the dtd from one is repeated in the second. Maybe we should file a bug to clean that up, it would be a candidate for contributors.

Comment 16

[Andreea Matei [:AndreeaMatei]]

This has been covered already and until the removal happens for landing, no need to keep as P1.

Comment 17

[Andrei Eftimie]

Unassigning. We'll revisit when the actual changes are done.

Comment 18

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

Panorama will be dead soon - and Mozmill tests are dead for a while now. Nothing we want to spend any time in getting it updated or removed.