1036848 - Fix location of shared modules (/firefox/lib vs. /lib)

Status: RESOLVED FIXED

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

Description

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

With the refactoring of the repository, some libraries were accidentally moved over to the Firefox library. We should revert that. So libraries included here are:

endurance.js
localization.js
modal-dialog.js
performance.js
screenshots.js
utils.js
widgets.js

Given that lots of code we will work on soon, will depend on it we should do this next. Daniel, could you work on fixing that?

Comment 1

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #0)
> Given that lots of code we will work on soon, will depend on it we should do
> this next. Daniel, could you work on fixing that?

Sure!

Comment 2

[Daniel Gherasim]

Attachment: v1.patch

http://mozmill-crowd.blargon7.com/#/functional/report/b4d880bccb8d9ea0732eac8cd8571800
http://mozmill-crowd.blargon7.com/#/remote/report/b4d880bccb8d9ea0732eac8cd857a9dc
http://mozmill-crowd.blargon7.com/#/endurance/report/b4d880bccb8d9ea0732eac8cd85831c4

Comment 3

[Andrei Eftimie]

Comment on attachment 8453742 [details] [diff] [review]
v1.patch

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

This looks fine to me.
We will need to land this across branches, and this will probably invalidate all(most) other patches that are in the works now.

I do wonder if we shouldn't better move _all_ libraries in the same place.

Comment 4

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

(In reply to Andrei Eftimie from comment #3)
> I do wonder if we shouldn't better move _all_ libraries in the same place.

How do you mean? You cannot have all the ui libraries which are targeted for Firefox to the top-level lib folder. You will always have global and product specific libraries.

Comment 5

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

Comment on attachment 8453742 [details] [diff] [review]
v1.patch

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

Looks mostly good. Some tweaks are necessary to do.

::: firefox/lib/search.js
@@ +12,5 @@
>  // Include required modules
>  var { assert, expect } = require("../../lib/assertions");
> +var modalDialog = require("../../lib/modal-dialog");
> +var utils = require("../../lib/utils");
> +var widgets = require("../../lib/widgets");

Widgets is ui, so better use /lib/ui/widgets.

::: firefox/lib/tabs.js
@@ +37,5 @@
>  
>  // Include required modules
>  var { assert } = require("../../lib/assertions");
>  var domUtils = require("../../lib/dom-utils");
> +var utils = require("../../lib/utils");

While you are on it fix the ordering.

::: lib/dom-utils.js
@@ -4,5 @@
>  
>  // Include required modules
>  var { assert, expect } = require("assertions");
> -var modalDialog = require("../firefox/lib/modal-dialog");
> -var utils = require("../firefox/lib/utils");

Great change. Remember that a global module should never include a product specific one. If that happens, something is broken!

::: lib/endurance.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var addons = require("../firefox/lib/addons");

Why can't we already move addons to /lib? Now that all is in-content, it is the same across products. What we still have to do is to split it up into a backend and ui module.

::: lib/utils.js
@@ +13,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  // Include required modules
> +var { assert, expect } = require("assertions");
> +var prefs = require("../firefox/lib/prefs");

Please file a follow-up bug, so we can get the prefs module separated into a backend and ui one. Backend code should move into a global prefs module. Or feel free to do it right now.

Comment 6

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) [away 07/19 - 08/01] from comment #4)
> (In reply to Andrei Eftimie from comment #3)
> > I do wonder if we shouldn't better move _all_ libraries in the same place.
> 
> How do you mean? You cannot have all the ui libraries which are targeted for
> Firefox to the top-level lib folder. You will always have global and product
> specific libraries.

Just as we used to have them before Metro.

(Yes, I know you'll reply that we can't do that because we have to support Metro - even if we actually don't - and that we _may_ have different products in the future. I would argue that we should do things _when_ we'll need them and no carry around cruft with us we don't actually need/use on a daily basis.)

Anyway, as I said, i was only _wondering_, so just food for thought.

Comment 7

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

(In reply to Andrei Eftimie from comment #6)
> (Yes, I know you'll reply that we can't do that because we have to support
> Metro - even if we actually don't - and that we _may_ have different
> products in the future. I would argue that we should do things _when_ we'll
> need them and no carry around cruft with us we don't actually need/use on a
> daily basis.)

Right, we are not going to revert it. We have the application as top-level folder in the mozmill-tests repository, and also adjusted our automation scripts and mozmill-ci code for this move. Given that the libarary files have to stay at the correct location.

Comment 8

[Daniel Gherasim]

Attachment: v2.patch

Filed bug 1040610 to split/refactor the prefs library & bug 1040605 for the addons.

Made the changes based on Henrik's review.

Comment 9

[Andrei Eftimie]

Comment on attachment 8460849 [details] [diff] [review]
v2.patch

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

Since this is such a big update, please run each of the testruns we have once to make sure everything works fine.

::: firefox/lib/tests/testAddonsManager.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var addons = require("../../../addons");

This path is not good.

::: lib/endurance.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var addons = require("../firefox/lib/addons");

You forgot to update this path.

::: lib/utils.js
@@ +13,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  // Include required modules
> +var { assert, expect } = require("assertions");
> +var prefs = require("../firefox/lib/prefs");

Please include the comment from addons.js for all general libraries that include product-specific ones.

Comment 10

[Andrei Eftimie]

Comment on attachment 8460849 [details] [diff] [review]
v2.patch

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

Since this is such a big update, please run each of the testruns we have once to make sure everything works fine.

lib/tests/manifest.ini is missing some files, please also check the rest of the lib manifest files.

::: firefox/lib/tests/testAddonsManager.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var addons = require("../../../addons");

This path is not good.

::: lib/endurance.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var addons = require("../firefox/lib/addons");

You forgot to update this path.

::: lib/utils.js
@@ +13,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  // Include required modules
> +var { assert, expect } = require("assertions");
> +var prefs = require("../firefox/lib/prefs");

Please include the comment from addons.js for all general libraries that include product-specific ones.

Comment 11

[Daniel Gherasim]

Attachment: v3.patch

http://mozmill-crowd.blargon7.com/#/endurance/report/533154d2e18fa05bbdf2eaada8715ab2
http://mozmill-crowd.blargon7.com/#/functional/report/533154d2e18fa05bbdf2eaada871578b
http://mozmill-crowd.blargon7.com/#/remote/report/533154d2e18fa05bbdf2eaada8715862

Comment 12

[Andrei Eftimie]

Comment on attachment 8464550 [details] [diff] [review]
v3.patch

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

Looks good other than the mentioned manifest file.

::: lib/tests/manifest.ini
@@ +1,2 @@
> +[testFormData.js]
> +[testPerformance.js]

This is good now, but you forgot to remote this test from firefox/lib/tests/manifest.ini
Please also make sure that the manifest file contains the correct tests.

Comment 13

[Daniel Gherasim]

Attachment: v4.patch

Comment 14

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

(In reply to daniel.gherasim from comment #11)
> http://mozmill-crowd.blargon7.com/#/endurance/report/
> 533154d2e18fa05bbdf2eaada8715ab2
> http://mozmill-crowd.blargon7.com/#/functional/report/
> 533154d2e18fa05bbdf2eaada871578b
> http://mozmill-crowd.blargon7.com/#/remote/report/
> 533154d2e18fa05bbdf2eaada8715862

I miss testruns for updates and addons here. Please do so.

Comment 15

[Daniel Gherasim]

Update:
http://mozmill-crowd.blargon7.com/#/update/report/ff9854eb4c07409123869856273f8c55
http://mozmill-crowd.blargon7.com/#/update/report/ff9854eb4c07409123869856273f8210

Addons - tested on release (had to fix patch to apply):
http://mozmill-crowd.blargon7.com/#/addons/report/ff9854eb4c07409123869856273fc197

Comment 16

[Daniel Gherasim]

Attachment: v4.1.patch

Fixed to apply cleanly.

Comment 17

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

Comment on attachment 8469158 [details] [diff] [review]
v4.1.patch

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

Given that all tests are passing, you get my review. I can't find anything else which would need tweaking. Also good to hear that the patch nearly applies cleanly to all branches. So lets get it in.

Comment 18

[Andrei Eftimie]

Comment on attachment 8469158 [details] [diff] [review]
v4.1.patch

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

Please update the patch since it doesn't apply cleanly anymore

Comment 19

[Daniel Gherasim]

Attachment: v4.2.patch

Patch for default & aurora.

Comment 20

[Andrei Eftimie]

Comment on attachment 8472235 [details] [diff] [review]
v4.2.patch

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

Looks good.
Landed: https://hg.mozilla.org/qa/mozmill-tests/rev/5808377acbb7 (default)

Comment 21

[Daniel Gherasim]

Attachment: beta.patch

http://mozmill-crowd.blargon7.com/#/remote/report/382887baef7652ce71ad45a3076c285e
http://mozmill-crowd.blargon7.com/#/functional/report/382887baef7652ce71ad45a3076c1bfb

Comment 22

[Andrei Eftimie]

Looks fine on Aurora as well, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/2f76117746f5 (mozilla-aurora)

With this affecting lots of files we should land it across branches as soon as possible to not block other patches. This will affect almost all other patches.

Comment 23

[Andrei Eftimie]

Comment on attachment 8472244 [details] [diff] [review]
beta.patch

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

You missed to update the assertions lib in performance.js, this fails all endurance tests.

Comment 24

[Daniel Gherasim]

Attachment: beta.patch

Comment 25

[Daniel Gherasim]

Attachment: release.patch

http://mozmill-crowd.blargon7.com/#/endurance/report/382887baef7652ce71ad45a30770460f
http://mozmill-crowd.blargon7.com/#/remote/report/382887baef7652ce71ad45a3076f18c4
http://mozmill-crowd.blargon7.com/#/functional/report/382887baef7652ce71ad45a3076ea2c0

Comment 26

[Andrei Eftimie]

Comment on attachment 8472276 [details] [diff] [review]
beta.patch

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

Looks good, ran functional, remote, update, endurance.
Landed: https://hg.mozilla.org/qa/mozmill-tests/rev/914c122ed957 (mozilla-beta)

Comment 27

[Andrei Eftimie]

Comment on attachment 8472278 [details] [diff] [review]
release.patch

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

All green on release as well.
https://hg.mozilla.org/qa/mozmill-tests/rev/1788e00f7aeb (mozilla-release)
https://hg.mozilla.org/qa/mozmill-tests/rev/bfed43ecd26a (mozilla-esr31)

Comment 28

[Andrei Eftimie]

I don't see a reason to push this on ESR24 since that branch won't live much longer.

Comment 29

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

Andrei, we had this topic already yesterday. But looks like you pushed the patch across all the branches again on a single day. IMO this is still too risky, and we really really should avoid that! Especially when we do this for beta and release branches. The risk here is even higher. So please stop such landings for now, and get a discussion started on the mailing list so we can agree on the right process and note that down. Thanks.

Comment 30

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) [away 08/16 - 08/23] from comment #29)
> Andrei, we had this topic already yesterday. But looks like you pushed the
> patch across all the branches again on a single day. IMO this is still too
> risky, and we really really should avoid that! Especially when we do this
> for beta and release branches. The risk here is even higher. So please stop
> such landings for now, and get a discussion started on the mailing list so
> we can agree on the right process and note that down. Thanks.

In general yes, but for particular cases it is worth to break this rule.
Since this touches a lot of files it would block backporting for _all_ other patches.

Daniel is on PTO today and next week. Tomorrow we have a bank holiday and no one will be at the office. You will be away next week. If we wouldn't have landed it yesterday, I would be hesitant to land it today because the next 3 days we'll have limited availability. It would have postponed it for next week when Daniel wasn't around.

Since we pushed it yesterday we have today to check that everything is working fine and fix if anything is broken. Because it does touch a lot of files the _risk_ is greater for something to break. That's why me and Daniel ran _lots_ of tests yesterday against all branches.

I did mention I want to land it across branches yesterday in comment 22 (I only mentioned ASAP there, not the specific day) and I mentioned on IRC _before_ going for Beta and lower that I want to and will push them.