602365 - Update shared modules use CommonJS

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

With CommonJS supported by Mozmill 1.5.1 (bug 588682) we will have to update all of our shared modules and tests. This task is planned after a week of using Mozmill 1.5.1 on our boxes in the QA lab. Once we can make sure that there is no regression we can begin with the update. It's quite a bit of work which is involved to get this bug fixed. So we will have to divide the work into groups and assign areas to different people. That way we should be able to do it in two days for the default branch. Backports have to follow.

Steps:

* I will update all shared modules so they will use CommonJS internally. No test should be affected by this change, because the module name should stay intact. It could be also useful for future usage.

* Once this has been finished we have to assign a number of subfolders to each person of the team. So we can independently update all the tests, get them reviewed, and checked-in.

The changes which will be introduced in this bug are the base for our upcoming shared module refactoring work. That means they are really important.

For now I will test the Common JS usage and will upload a WIP patch for the shared modules hopefully by tomorrow.

Comment 1

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

Attachment: Patch v1 (part I - shared modules)

Initial patch for all of our shared modules. Internally they will be able to use CommonJS while the tests itself will use the old module collector.

After all tests have been updated, we will have to rename the shared modules and give them a better name. I propose something like "testUtilsAPI.js" -> "utils.js".

Geo, please have a look at the patch. You will not be able to execute the tests with a release build of mozmill but the latest dev version from hotfix-1.5.1 should work fine.

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 481706 [details] [diff] [review]
Patch v1 (part I - shared modules)

Patch looks pretty good so far.  It's a lot to review all at once, though, so you might want to consider splitting it up some.  Other than that, carry on!

Comment 3

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

(In reply to comment #2)
> Patch looks pretty good so far.  It's a lot to review all at once, though, so
> you might want to consider splitting it up some.  Other than that, carry on!

Well this is a combined patch. I can't split this up because of the dependencies between the shared modules. Once we update the tests we will have to attach patches for each sub folder separately.

Comment 4

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Fair enough.  Figured I'd throw it out there in hopes that even shared modules changes could be chunked a little.  

I figured since the module vars are only used internally, there was no real reason they couldn't be handled one-by-one, but maybe it's the external module identifier that's the problem.  OTOH, we could have chosen not to remove that yet so the patch could broken up.  :)

Comment 5

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

Attachment: Patch v1.1 (part I - shared modules)

This patch updates the changes to the current head and also includes the tabView API.

Comment 6

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

Attachment: Patch v1.2 (part I - shared modules)

Sorry, forgot about the exports for the TabViewAPI

Comment 7

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

Attachment: Patch v1.3 (part I - shared modules)

Given my talk with Geo I made some small tweaks.

Comment 8

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

Attachment: Patch v1.4 (part I - shared modules) [checked-in]

Figured that there is a spelling error in the UtilsAPI which breaks some of the tests. Lets get it fixed too.

Comment 9

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

Lets keep this bug for the shared module work only. Update for tests should be worked on separate bugs.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 485857 [details] [diff] [review]
Patch v1.4 (part I - shared modules) [checked-in]

LGTM, don't see any further problems. Go ahead and land it and we can get started with test patches!

Comment 11

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

Comment on attachment 485857 [details] [diff] [review]
Patch v1.4 (part I - shared modules) [checked-in]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/a7d0b8557161

Comment 12

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

Attachment: Patch v1 (part I - shared modules - 1.9.2)

Backport for 1.9.2 branch.

Comment 13

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

Attachment: Patch v1 (part I - shared modules - 1.9.1)

Backport for 1.9.1 branch.

Comment 14

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

Attachment: Patch v1.1 (part I - shared modules - 1.9.2)

Missed the exports in the addons API.

Comment 15

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

Attachment: Patch v1.1 (part I - shared modules - 1.9.1)

Same for the API in 1.9.1

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 486024 [details] [diff] [review]
Patch v1.1 (part I - shared modules - 1.9.2)

LGTM, please land when the branch opens

Comment 17

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 486025 [details] [diff] [review]
Patch v1.1 (part I - shared modules - 1.9.1)

Also LGTM, please land when branch opens.

Comment 18

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

Geo, can you also please file a bug for your section? Would be nice to have all the stuff checked into default by tomorrow.

Comment 19

[Aaron Train [:aaronmt]]

Raising the issue in this bug as I suspect it a commonJS conversion issue. I've noticed [1] and seen in local test runs that there's an issue getting the collector, particularly in this call at [2]

|var prefs = collector.getModule('PrefsAPI').preferences;|

[1] http://mozmill-release.brasstacks.mozilla.com/#/general/report/4a2217a8a22f02aec478e6684a15525a

[2] http://hg.mozilla.org/qa/mozmill-tests/file/390d7b2f8e20/shared-modules/testUtilsAPI.js#l294

This is affecting those three security tests: safeBrowsingNotificationBar, safeBrowsingWarningPages, and testUntrustedConnectionErrorPage

Comment 20

[Aaron Train [:aaronmt]]

Attachment: testUtilsAPI breakage fix (default) [checked-in]

Comment 21

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

Comment on attachment 486452 [details] [diff] [review]
testUtilsAPI breakage fix (default) [checked-in]

>+// Load the required modules
>+var prefs = require("testPrefsAPI");

Good catch! Strange that I have missed that!

The comment should be similar to all the other comments in the tests. I have seen that I used the wrong comments in the shared modules. I will update it in one step.

>-  var prefValue = prefs.getPref("browser.startup.homepage", "",
>+  var prefValue = preferences.getPref("browser.startup.homepage", "",
>                                 true, Ci.nsIPrefLocalizedString);

I will also correct the indentation.

Comment 22

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

Comment on attachment 486452 [details] [diff] [review]
testUtilsAPI breakage fix (default) [checked-in]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/c41f40e28560

Comment 23

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

Attachment: Patch v2 (part I - shared modules - 1.9.2) [checked-in]

Updated backport of shared modules for 1.9.2 with missing parts.

Comment 24

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

Attachment: Patch v2 (part I - shared modules - 1.9.1) [checked-in]

Updated backport of shared modules for 1.9.1 with missing parts.

Comment 25

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

Attachment: Final patch for shared modules v1 (default) [checked-in]

That's the final patch which completely cleans-up all the old crufted code in favor of CommonJS. Yay!

Comment 26

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 486650 [details] [diff] [review]
Final patch for shared modules v1 (default) [checked-in]

r+ pending renames of the files, but I'm mildly surprised by this considering we hadn't discussed renaming the files yet.

Comment 27

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 486475 [details] [diff] [review]
Patch v2 (part I - shared modules - 1.9.1) [checked-in]

Looks fine.

Comment 28

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 486469 [details] [diff] [review]
Patch v2 (part I - shared modules - 1.9.2) [checked-in]

Looks fine.

Comment 29

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

> r+ pending renames of the files, but I'm mildly surprised by this considering
> we hadn't discussed renaming the files yet.

Discussed rename with whimboo, let's do it, r+.

Comment 30

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

Comment on attachment 486650 [details] [diff] [review]
Final patch for shared modules v1 (default) [checked-in]

Now we have completed the CommonJS transition on default. Branches will follow soon.

Comment 31

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

Comment on attachment 486469 [details] [diff] [review]
Patch v2 (part I - shared modules - 1.9.2) [checked-in]

Landed on 1.9.2 as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d2f9cefb2202

Comment 32

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

Comment on attachment 486475 [details] [diff] [review]
Patch v2 (part I - shared modules - 1.9.1) [checked-in]

Landed on 1.9.1 as:
http://hg.mozilla.org/qa/mozmill-tests/rev/635eee9055b5

Comment 33

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

Backports landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/716a1229a3d9 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/129c9b95ad59 (1.9.1)

Comment 34

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

Sorry, that was the wrong bug.

Comment 35

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

(In reply to comment #30)
> Comment on attachment 486650 [details] [diff] [review]
> Final patch for shared modules v1 (default) [checked-in]

This has landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/aca4ffe102bb

Comment 36

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

Attachment: Final patch for shared modules v1 (1.9.2)

Had to mostly do it from scratch, because some files we have on the default branch don't exist in the older branches and that screwed everything up. :/

I ran a complete test-run and everything looks fine. No JS errors appear.

Comment 37

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

Attachment: Final patch for shared modules v1 (1.9.1)

This patch also fixes a problem in testSafeBrowsingNotificationBar.js where we had a missing closing bracket and which caused a JS error.

Comment 38

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 486880 [details] [diff] [review]
Final patch for shared modules v1 (1.9.2)

Looks fine, please land.

Comment 39

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 486885 [details] [diff] [review]
Final patch for shared modules v1 (1.9.1)

Looks fine, please land.

Comment 40

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

Final patches for older branches landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/8b0c4b58c592 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/d4d762861c40 (1.9.1)

We are done! It's great to see CommonJS in place!

Comment 41

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

Comment on attachment 486475 [details] [diff] [review]
Patch v2 (part I - shared modules - 1.9.1) [checked-in]

>+// Export of classes
>+exports.softwareUpdate = softwareUpdate;
>+exports.softwareUpdate.prototype = softwareU

My backport patch (part I) for 1.9.1 has introduced this broken line at the end of the software update module. That's the reason why update tests were broken on 1.9.1. I will fix this with the patch to land on bug 608511.

Comment 42

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (Bustage fix for tabs reference in addons API)

Comment 43

[Aaron Train [:aaronmt]]

(In reply to comment #42)
> Created attachment 490609 [details] [diff] [review]
> Patch v1 - (Bustage fix for tabs reference in addons API)

http://hg.mozilla.org/qa/mozmill-tests/rev/9a0629f1bd44

Comment 44

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (Followup: Another bad reference in addons API)

Comment 45

[Aaron Train [:aaronmt]]

(In reply to comment #44)
> Created attachment 490649 [details] [diff] [review]
> Patch v1 - (Followup: Another bad reference in addons API)

http://hg.mozilla.org/qa/mozmill-tests/rev/328f78ba60aa

Comment 46

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.