982610 - Update TPS to use latest Mozmill 2.0.6

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

When TPS has been created the included Mozmill version was in alpha state. Now that Mozmill 2.0 has been released and even Mozmill 2.0.6 is available, we should update TPS to make use of that version for stability.

https://github.com/mozilla/mozmill/tree/2.0.6

It is not that urgent as to get all the tests running, so we can delay that a bit.

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 0001-Update-mozmill-extension-in-TPS-WIP-patch.patch

This is a WIP patch:
I had to add the extension code from mozmill in to TPS.
To remove jsbridge module loading in mozmill
Fix tests, they haven't been ran with australis
Fix other import errors.

I have some work to do here but I'm close, it looks like mozmill_sanity tests haven't been ran in a long time.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.1

With this the mozmill_sanity tests passes. It still has an issue with aModule, it looks like controller is not persisted between test modules, I will have to fix that tomorrow then the patch will be ready for review.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

The setupModule wasn't loading because when calling initTestModule, name parameter has to be null in oreder for setupModule to ran, in our case it was undefined.
> https://github.com/mozilla/mozmill/blob/hotfix-2.0/mozmill/mozmill/extension/resource/modules/frame.js#L468

I will update a patch in which I will just copy paste the mozmill extension so you can make a diff between the patches, to see only the changes I made, for the extension to work.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 0001-PATCH-FOR-DIFF.patch

This patch is for making a diff between just coping the mozmill extension in TPS.

Comment 6

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

(In reply to Cosmin Malutan from comment #3)
> The setupModule wasn't loading because when calling initTestModule, name
> parameter has to be null in oreder for setupModule to ran, in our case it
> was undefined.
> > https://github.com/mozilla/mozmill/blob/hotfix-2.0/mozmill/mozmill/extension/resource/modules/frame.js#L468

Why it is undefined? And what did you change? I really don't get that. Please be more specific in explaining changes you do.

> I will update a patch in which I will just copy paste the mozmill extension
> so you can make a diff between the patches, to see only the changes I made,
> for the extension to work.

That's not necessary. Bugzilla supports interdiff, so you might want to give me that link and the appropriate lines for the changes.

Comment 7

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

Comment on attachment 8426170 [details] [diff] [review]
patch_v2.1

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

::: services/sync/tests/tps/mozmill_sanity2.js
@@ -1,2 @@
> -/* Any copyright is dedicated to the Public Domain.
> -   http://creativecommons.org/publicdomain/zero/1.0/ */

MPL 2 please.

@@ +7,4 @@
>    controller.open("about:support");
>    controller.waitForPageLoad();
>  
> +  var appbox = new findElement.ID(controller.tabs.activeTab, "application-box");

findElement.ID is a method and doesn't need the new operator. I already told this on another bug.

@@ +7,5 @@
>    controller.open("about:support");
>    controller.waitForPageLoad();
>  
> +  var appbox = new findElement.ID(controller.tabs.activeTab, "application-box");
> +  assert.waitFor(() => appbox.getNode().innerHTML == 'Firefox', 'correct app name');

I don't see why we need a waitFor here.

@@ +47,2 @@
>    controller.click(item);
> +  item = new findElement.Lookup(controller.window.document, SEARCH_AUTOCOMPLETE);

I would remove all those tests. I don't see how those are useful.

::: services/sync/tps/extensions/mozmill/build.xml
@@ +21,5 @@
> +  <target name="cleanup">
> +    <!-- Delete the chrome directory, any other cleanup actions go here -->
> +    <delete dir="chrome"/>
> +  </target>
> +</project>

We do not need this file.

::: services/sync/tps/extensions/mozmill/components/handlers.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */

Why are you adding this file? It's not necessary and will only fail given that it is using jsbridge all over the place.

::: services/sync/tps/extensions/mozmill/content/test/chrome_elements.xul
@@ +1,2 @@
> +<?xml version="1.0"?>
> +

only used for mutt tests, which we don't need here.

::: services/sync/tps/extensions/mozmill/content/test/radio_buttons.xul
@@ +1,2 @@
> +<?xml version="1.0"?>
> +

Same for that file, and others under that folder.

::: services/sync/tps/extensions/mozmill/defaults/preferences/debug.js
@@ -3,5 @@
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -
> -/* debugging prefs */
> -pref("browser.dom.window.dump.enabled", true);
> -pref("javascript.options.showInConsole", true);

Are you sure it can be removed? I think we would fail to dump to the console. Or is that set via the testrunner itself already?

::: services/sync/tps/extensions/mozmill/install.rdf
@@ -1,4 @@
>  <?xml version="1.0"?>
> -<!-- This Source Code Form is subject to the terms of the Mozilla Public
> -   - 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/. -->

Why the removal?

::: services/sync/tps/extensions/tps/resource/tps.jsm
@@ -39,5 @@
>  var prefs = Cc["@mozilla.org/preferences-service;1"]
>              .getService(Ci.nsIPrefBranch);
>  
> -var mozmillInit = {};
> -Cu.import('resource://mozmill/modules/init.js', mozmillInit);

Why do we remove it here but not replace it with mozmill.js? As of now we wont initialize Mozmill correctly.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.0

(In reply to Henrik Skupin (:whimboo) from comment #7)
> > +  var appbox = new findElement.ID(controller.tabs.activeTab, "application-box");
> > +  assert.waitFor(() => appbox.getNode().innerHTML == 'Firefox', 'correct app name');
> 
> I don't see why we need a waitFor here.
This is asynchronously updated so it's not ready when the pageload event is dispatched:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js#35

> > -/* debugging prefs */
> > -pref("browser.dom.window.dump.enabled", true);
> > -pref("javascript.options.showInConsole", true);
> 
> Are you sure it can be removed? I think we would fail to dump to the
> console. Or is that set via the testrunner itself already?
I can make dumps without this prefs, those were removed wen we passed from mozmill 1.5 to 2.0

Comment 9

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

(In reply to Cosmin Malutan from comment #8)
> > > +  var appbox = new findElement.ID(controller.tabs.activeTab, "application-box");
> > > +  assert.waitFor(() => appbox.getNode().innerHTML == 'Firefox', 'correct app name');
> > 
> > I don't see why we need a waitFor here.
> This is asynchronously updated so it's not ready when the pageload event is
> dispatched:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.
> js#35

Then also use textContent as the code you are referring here is doing. innerHTML is dangerous!

> > > -/* debugging prefs */
> > > -pref("browser.dom.window.dump.enabled", true);
> > > -pref("javascript.options.showInConsole", true);
> > 
> > Are you sure it can be removed? I think we would fail to dump to the
> > console. Or is that set via the testrunner itself already?
> I can make dumps without this prefs, those were removed wen we passed from
> mozmill 1.5 to 2.0

Just checked and those prefs are getting set in testrunner.py.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.1

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Then also use textContent as the code you are referring here is doing.
> innerHTML is dangerous!
Done it!

Comment 11

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

Comment on attachment 8426895 [details] [diff] [review]
patch_v3.1

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

This looks fine to me. Thanks Cosmin. I also tested it, and the sanity tests for Mozmill are passing now. I will get this landed in a bit.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d230cf0dab

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f6d230cf0dab

Comment 14

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

We will get this tested today, and if all is stable we can do the backports. The patch should apply on all affected branches.

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e33cb9495db
https://hg.mozilla.org/releases/mozilla-beta/rev/c32d46efa757

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c32d46efa757